Srvb preview html#181
Conversation
The collection tag contains entity sets and associations and it is a list.
To help with the future feature "preview html".
The feature was proposed by Michael Hoerisch. I am not sure if it would it make more sense to make open browser default but all the other command prints something to terminal.
📝 WalkthroughWalkthroughAdds a new ChangesSRVB Preview HTML Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as sap.cli.srvb
participant ADT as ODataV4ServiceGroup
participant Connection
User->>CLI: srvb preview html --service ...
CLI->>ADT: fetch service binding/group
ADT-->>CLI: service_information, entity sets
CLI->>CLI: _encode_feap_path_params(...)
alt --open flag
CLI->>User: webbrowser.open(url)
else default
CLI->>Connection: execute('GET', path, params)
Connection-->>CLI: html text
CLI->>User: print(html)
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/unit/test_sap_cli_srvb.py (1)
389-397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFlake8 E126: over-indented continuation line.
Static analysis flags these continuation lines as over-indented hanging indents.
Also applies to: 448-456
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/test_sap_cli_srvb.py` around lines 389 - 397, The response fixture formatting in the test setup has over-indented hanging continuation lines that trigger Flake8 E126. Reformat the multi-line Response(...) calls in the affected test blocks so the indentation of the continued arguments is consistent and compliant, including the similar fixture setup near the other referenced block; keep the logic unchanged and only adjust the indentation in the test helper data.Source: Linters/SAST tools
sap/cli/srvb.py (2)
275-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBespoke path-encoding scheme could use a named constant and a note on the separator assumption.
The +20 shift is a magic number, and the
##separator is not validated against appearing insideservice_url/entity_set/etc. Both are functionally verified by fixtures today, but a small constant plus a defensive comment would make this reverse-engineered encoding easier to maintain if SAP's own scheme changes.♻️ Optional: extract the shift as a named constant
+# Code-point shift used by ADT's own web frontend when building the +# encoded `feap/.../flp.html` path fragment. +_FEAP_PATH_CODEPOINT_SHIFT = 20 + def _encode_feap_path_params(service_url, entity_set, service_name, service_version, group_name): ... components = [service_url, entity_set, '', '', service_name, service_version, group_name] params = "##".join(components) - encodedparams = "".join([chr(b) for b in [ord(c) + 20 for c in params]]) + encodedparams = "".join(chr(ord(c) + _FEAP_PATH_CODEPOINT_SHIFT) for c in params) return urllib.parse.quote(encodedparams)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sap/cli/srvb.py` around lines 275 - 287, The _encode_feap_path_params helper uses a magic +20 character shift and assumes the "##" separator will not appear in any component, which makes the reverse-engineered encoding harder to maintain. Update this function by extracting the shift into a clearly named constant and adding a brief defensive comment near the components/join logic in _encode_feap_path_params explaining the separator assumption and that it matches the ADT-derived format.
323-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a shared helper for the preview URL The
--openbranch still hand-concatenates the browser URL fromconnection._http_client._base_urlandpath, while the non-browser path goes throughconnection.execute(), which uses_build_adt_url(). A public preview-URL helper onConnectionwould keep both paths on one source of truth and avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sap/cli/srvb.py` around lines 323 - 341, The preview URL is being assembled manually in the `args.open_in_browser` branch instead of using the same shared logic as `connection.execute()`, which can cause the browser path to drift. Add a public preview-URL helper on `Connection` (or reuse `_build_adt_url()` through a wrapper) and update `srvb.py` to call that helper when building the `full_url`, keeping both preview flows on one source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@sap/cli/srvb.py`:
- Around line 275-287: The _encode_feap_path_params helper uses a magic +20
character shift and assumes the "##" separator will not appear in any component,
which makes the reverse-engineered encoding harder to maintain. Update this
function by extracting the shift into a clearly named constant and adding a
brief defensive comment near the components/join logic in
_encode_feap_path_params explaining the separator assumption and that it matches
the ADT-derived format.
- Around line 323-341: The preview URL is being assembled manually in the
`args.open_in_browser` branch instead of using the same shared logic as
`connection.execute()`, which can cause the browser path to drift. Add a public
preview-URL helper on `Connection` (or reuse `_build_adt_url()` through a
wrapper) and update `srvb.py` to call that helper when building the `full_url`,
keeping both preview flows on one source of truth.
In `@test/unit/test_sap_cli_srvb.py`:
- Around line 389-397: The response fixture formatting in the test setup has
over-indented hanging continuation lines that trigger Flake8 E126. Reformat the
multi-line Response(...) calls in the affected test blocks so the indentation of
the continued arguments is consistent and compliant, including the similar
fixture setup near the other referenced block; keep the logic unchanged and only
adjust the indentation in the test helper data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5bb035f-a083-4f4d-97f0-686eadc1994e
📒 Files selected for processing (6)
doc/commands/srvb.mdsap/adt/businessservice.pysap/cli/srvb.pytest/unit/fixtures_adt_businessservice.pytest/unit/test_sap_adt_businessservice.pytest/unit/test_sap_cli_srvb.py
|
Live-verified against an S/4HANA Public Cloud tenant (DDCI-flavor) on 2026-07-06. Combined with #182 into one test install.
That alone closes a small papercut — before this PR the caller had to
URL construction is byte-identical to what ADT itself generates. I had reverse-engineered the FEAP plaintext layout independently on the same tenant (via decoding an ADT-generated URL with the inverse of Byte-for-byte match. The Caesar-shift-by-20 + URL-encode step is confirmed against the server-side decoder in the ABAP class Error paths behave cleanly:
Great feature. Displaces a ~50-line manual URL-construction Python script we had been carrying as a plugin skill. |
3 participants
No description provided.