Conversation
Port the extensibility customization guide from the Orders/bookshop sample to XTravels: model-only extension (x_priority + x_CostCenter), multi-repo workspace setup, local multitenancy, and tenant subscription. Regenerate all four screenshots against a running XTravels tenant: - base Travels list - extension project readme - Fiori preview with Priority and Cost Center columns - deployed tenant UI with Priority and Cost Center columns Remove the superseded Orders screenshots.
|
👋 Hi — I'm PR Bot, your SAP code review assistant. I'll automatically review your pull requests for code quality, security, and SAP compliance. Get an overview of what I do → What I do
Key commands
*This introduction message will be shown to you only once, you will not see it in future PRs. |
Control PanelHi, I'm an AI-powered Review Bot that helps you with summarizing and reviewing pull requests.
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
I posted several focused review comments on changed lines covering a correctness issue in the verification flow, a misleading CodeList statement, a typo, and accessibility improvements for alt text and sensory-dependent instructions. The guide is generally coherent after the XTravels port, but these fixes should improve accuracy and reader usability before merge.
PR Bot Information
Version: 1.31.51
- Review Focus Files:
- LLM:
gpt-5.5 - File Content Strategy: Full file content
- Correlation ID:
ec556a80-b755-11f1-9702-c9dd4a88854f - Event Trigger:
issue_comment.edited
| 2. Verify that it worked by opening the [XTravels Fiori UI](http://localhost:4004/travels/webapp/index.html) in a **new private browser window** and log in as `carol`, which is assigned to tenant `t1`. | ||
|
|
||
| {.mute-dark} | ||
| {.mute-dark} |
There was a problem hiding this comment.
Best Practices: The alt text describes the image type instead of the image content
Alt text should summarize the useful information and avoid phrases such as “A screenshot” and “It shows.” Consider making the table content the subject of the alt text.
| {.mute-dark} | |
| {.mute-dark} |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| ``` | ||
|
|
||
| {.ignore-dark} | ||
| {.ignore-dark} |
There was a problem hiding this comment.
Best Practices: The alt text does not summarize the informative content
Alt text should describe the purpose or key message of the screenshot rather than referring to the screenshot itself. Consider naming the README content that the reader should take away.
| {.ignore-dark} | |
| {.ignore-dark} |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| #### Verify the Extension | ||
|
|
||
| Verify your extensions are applied correctly by opening the [Orders Fiori Preview](http://localhost:4006/$fiori-preview/OrdersService/Orders#preview-app) in a **new private browser window**, log in as `bob`, and see columns _Priority_ and _Sales Region_ filled as in the following screenshot: | ||
| Verify your extensions are applied correctly by opening the [Travels Fiori Preview](http://localhost:4006/$fiori-preview/TravelService/Travels#preview-app) in a **new private browser window**, log in as `bob`, and see columns _Priority_ and _Cost Center_ filled as in the following screenshot: |
There was a problem hiding this comment.
Best Practices: The instruction relies on sensory language and combines several actions
“See columns” is less accessible and the sentence is hard to follow. Consider using “confirm” and keeping the expected result clear.
| Verify your extensions are applied correctly by opening the [Travels Fiori Preview](http://localhost:4006/$fiori-preview/TravelService/Travels#preview-app) in a **new private browser window**, log in as `bob`, and see columns _Priority_ and _Cost Center_ filled as in the following screenshot: | |
| Verify your extensions are applied correctly by opening the [Travels Fiori Preview](http://localhost:4006/$fiori-preview/TravelService/Travels#preview-app) in a **new private browser window**, log in as `bob`, and confirm that the _Priority_ and _Cost Center_ columns contain values, as in the following screenshot: |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| Verify your extensions are applied correctly by opening the [Travels Fiori Preview](http://localhost:4006/$fiori-preview/TravelService/Travels#preview-app) in a **new private browser window**, log in as `bob`, and see columns _Priority_ and _Cost Center_ filled as in the following screenshot: | ||
|
|
||
| {.mute-dark} | ||
| {.mute-dark} |
There was a problem hiding this comment.
Best Practices: The alt text starts with screenshot boilerplate
Alt text should convey the relevant state directly and avoid “A screenshot” or “shows.” Consider focusing on the Fiori preview and extension column values.
| {.mute-dark} | |
| {.mute-dark} |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| #### Verify the Extension {#test-extension } | ||
|
|
||
| Verify your extensions are applied correctly by opening the [Order Management UI](http://localhost:4004/orders/index.html#manage-orders) in a **new private browser window**, log in as `bob`, and check that columns _Priority_ and _Sales Region_ are displayed as in the following screenshot. Also, check that there's content with a proper label in the _Sales Region_ column. | ||
| Verify your extensions are applied correctly by opening the [XTravels UI](http://localhost:4004/travels/webapp/index.html) in a **new private browser window**, log in as `bob`, and check that columns _Priority_ and _Cost Center_ are displayed as in the following screenshot. Also, check that there's content with a proper label in the _Cost Center_ column. |
There was a problem hiding this comment.
Logic Error: This verification expects Cost Center data before the guide adds it
The next step says the Cost Center column is not filled until users copy initial data to db/data/, so readers who follow the steps in order will see a blank column here. Consider moving the Add Data step before this verification, or change this verification and screenshot to check only that the columns exist.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| ### Extending the Service Model | ||
|
|
||
| In the existing in `OrdersService`, the new entities `x_CustomerPostalAddresses` and `x_Remarks` are automatically included since they are targets of the corresponding _compositions_. | ||
| In the existing in `TravelService`, the new entities `x_ApproverPostalAddresses` and `x_Notes` are automatically included since they are targets of the corresponding _compositions_. |
There was a problem hiding this comment.
Typo: The sentence has an extra “in”
“In the existing in TravelService” is grammatically incorrect and interrupts the explanation. Consider making TravelService the location of the auto-included entities.
| In the existing in `TravelService`, the new entities `x_ApproverPostalAddresses` and `x_Notes` are automatically included since they are targets of the corresponding _compositions_. | |
| In `TravelService`, the new entities `x_ApproverPostalAddresses` and `x_Notes` are automatically included since they are targets of the corresponding _compositions_. |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| In the existing in `TravelService`, the new entities `x_ApproverPostalAddresses` and `x_Notes` are automatically included since they are targets of the corresponding _compositions_. | ||
|
|
||
| The new entities `x_Customers` and `x_SalesRegion` are [autoexposed](../services/providing-services#auto-exposed-entities) in a read-only way as [CodeLists](../../cds/common#aspect-codelist). Only if wanted to _change_ it, you would need to expose them explicitly: | ||
| The new entities `x_Approvers` and `x_CostCenters` are [autoexposed](../services/providing-services#auto-exposed-entities) in a read-only way as [CodeLists](../../cds/common#aspect-codelist). Only if wanted to _change_ it, you would need to expose them explicitly: |
There was a problem hiding this comment.
Bug: The text incorrectly says both entities are CodeLists
x_Approvers is defined with cuid, managed, while only x_CostCenters extends CodeList. Consider separating the autoexposure statement from the CodeList statement so readers do not model x_Approvers as a code list.
| The new entities `x_Approvers` and `x_CostCenters` are [autoexposed](../services/providing-services#auto-exposed-entities) in a read-only way as [CodeLists](../../cds/common#aspect-codelist). Only if wanted to _change_ it, you would need to expose them explicitly: | |
| The new entities `x_Approvers` and `x_CostCenters` are [autoexposed](../services/providing-services#auto-exposed-entities) in a read-only way. Only `x_CostCenters` is a [CodeList](../../cds/common#aspect-codelist). If you want to change how they are exposed, expose them explicitly: |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
Co-authored-by: hyperspace-pr-bot[bot] <209611008+hyperspace-pr-bot[bot]@users.noreply.github.com>
This branch has not been deployed
1 participant
What
Ports the extensibility customization guide from the Orders/bookshop sample to XTravels.
Changes
x_priority(enum high/medium/low) andx_CostCenter(association to a newx_CostCenterscode list), extending only app-localsap.capire.travelsentities.Notes