-
Notifications
You must be signed in to change notification settings - Fork 68
[HTTP] Add new object type HTTP #709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the AFF. I have a few suggestions :)
service_class TYPE zif_aff_types_v1=>ty_object_name_30, | ||
"! <p class="shorttext">URL</p> | ||
"! URL | ||
service_url TYPE zif_aff_types_v1=>ty_object_name_30, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type c length 60?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, should I use 'type c length 60'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the service url currently has the type zif_aff_types_v1=>ty_object_name_30, which should be used for fields which represent an object type.
Use type string, this way the url size is not limited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One follow up question from my side: How long can URLs be (or what length do you use in your database persistence)? In your first version it was 30 characters, now it's 60.
Take for example https://github.com/SAP/abap-file-formats/pull/709/files/d241275335d9e025121951bda910f77679b6203f#diff-fe5a33e190666260de7e040363387bf5643a7217edd3eee624e26096c39a7e73 - this is 166 characters long. Would that be a valid value for URL?
Or are valid values more like "/sap/bc/servicegroup/servicename" (32 characters)?
It very important to consider this now, as it is quite hard to change the length of this field in the future once this is in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it is too short.
The URL has always the format:
https://LDCIALX.WDF.SAP.CORP:44318/sap/bc/http/sap/30-charakter-long-name?sap-client=000
I increased the URL field to length 120, that should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now when I see the URL, one more question:
Is the URL something calculated or can the user define this one? Does it always follow the pattern:
https://ldci[SID].wdf.sap.corp:44318/sap/bc/http/sap/[OBJECT-NAME]?sap-client=000
or more general
https://[URI of ABAP AS] /sap/bc/http/sap/[OBJECT-NAME]?sap-client=000
?
Is this only intended for display or can the user actually change this? All fields in the AFF should be transported (and can be imported/exported via abapgit/gcts/...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL is calculated, as you wrote: https://[URI of ABAP AS]/sap/bc/http/sap/[OBJECT-NAME]?sap-client=XXX?
User can not change it, he can only define the object name in the wizzard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, in this case the URL cannot be part of the AFF, as it should only contain information that should be transported.
As you probably want to show the URL in the server-driven editor, maybe our ux colleagues have an idea how to show that best. @SAP/abap-file-formats-ux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well caught Markus. Let's finish the AFF review and then that issue can be look at during the UX Review. Let's leave this Thread open.
On your end @WUEHR , please delete the url field. We will find a more fitting solution to display this url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted the URL field. Not much left then in the UI :D
May I request a UX Review where we could discuss where to put the URL field?
No description provided.