[SUSI] Add new object type SUSI #526
Conversation
|
|
|
|
Co-authored-by: Katharina Wurz <[email protected]>
Co-authored-by: Katharina Wurz <[email protected]>
Co-authored-by: Katharina Wurz <[email protected]>
Co-authored-by: Katharina Wurz <[email protected]>
Co-authored-by: Katharina Wurz <[email protected]>
There was a problem hiding this comment.
Thanks, @WDFYvonne.
I went through the example file and added some comments and questions. Based on your feedback on these questions, I will review the other files, too.
Co-authored-by: Michael Schneider <[email protected]>
Co-authored-by: Michael Schneider <[email protected]>
|
Hallo Michael,
nachdem ich Deine Vorschläge übernommen habe, bekomm ich diesen Fehler:
***@***.***
Soll ich es wieder auf "general" setzen?
Viele Grüße,
Yvonne
From: Michael Schneider ***@***.***>
Sent: Wednesday, 26 April 2023 16:57
To: SAP/abap-file-formats ***@***.***>
Cc: Jasiewicz, Yvonne Barbara ***@***.***>; Mention ***@***.***>
Subject: Re: [SAP/abap-file-formats] [SUSI] Add new object type SUSI (PR #526)
@schneidermic0 requested changes on this pull request.
Thanks, @WDFYvonne<https://github.com/WDFYvonne>.
I went through the example file and added some comments and questions. Based on your feedback on these questions, I will review the other files, too.
________________________________
In file-formats/susi/examples/zaff_example_susi.susi.json<#526 (comment)>:
+ "originalLanguage": "en",
+ "abapLanguageVersion": "standard"
"abapLanguageVersion": "standard" is typically not serialized since this is the default value
⬇️ Suggested change
- "originalLanguage": "en",
- "abapLanguageVersion": "standard"
+ "originalLanguage": "en"
________________________________
In file-formats/susi/examples/zaff_example_susi.susi.json<#526 (comment)>:
@@ -0,0 +1,24 @@
+{
+ "formatVersion": "1",
+ "header": {
+ "description": "Example SUSI object",
+ "originalLanguage": "en",
+ "abapLanguageVersion": "standard"
+ },
+ "general": {
+ "applName": "Z_FEM3/Z_FEM3_GET_INFO",
+ "type": "WebService",
+ "hashName": "D2FF3796C3963799AE24F0224FCD96HS",
I am wondering wether the hashName is the actual object name, isn't it?
If so, we should name the example object's file name to D2FF3796C3963799AE24F0224FCD96HS.susi.json and remove it from the file's content.
________________________________
In file-formats/susi/examples/zaff_example_susi.susi.json<#526 (comment)>:
@@ -0,0 +1,24 @@
+{
+ "formatVersion": "1",
+ "header": {
+ "description": "Example SUSI object",
+ "originalLanguage": "en",
+ "abapLanguageVersion": "standard"
+ },
+ "general": {
for other object types we named this section "generalInformation". So for consistency reasons I suggest:
⬇️ Suggested change
- "general": {
+ "generalInformation": {
________________________________
In file-formats/susi/type/zif_aff_susi_v1.intf.abap<#526 (comment)>:
+ u_flag TYPE ty_maintenance_status VALUE 'U',
+ "! <p class="shorttext">Authorization Check Inactive</p>
+ "! Authorization Check Inactive
+ n_flag TYPE ty_maintenance_status VALUE 'N',
+ END OF co_maintenance_status.
+
+ TYPES:
+ "! <p class="shorttext">Application Name</p>
+ "! Name of Application
+ ty_appl_name TYPE c LENGTH 132,
+ "! <p class="shorttext">Hash value</p>
+ "! Hash Values for an Application's Auth. Default Values
+ ty_hash_name TYPE c LENGTH 32,
+ "! <p class="shorttext">Application Type</p>
+ "! Type of Application
+ ty_type TYPE c LENGTH 80.
Should there be an enum value for the possible values of type? In the example the value is "webService". Is this something the user enters?
________________________________
In file-formats/susi/examples/zaff_example_susi.susi.json<#526 (comment)>:
+ "formatVersion": "1",
+ "header": {
+ "description": "Example SUSI object",
+ "originalLanguage": "en",
+ "abapLanguageVersion": "standard"
+ },
+ "general": {
+ "applName": "Z_FEM3/Z_FEM3_GET_INFO",
+ "type": "WebService",
+ "hashName": "D2FF3796C3963799AE24F0224FCD96HS",
+ "maintenanceMode": "manual"
+ },
+ "authorizationObjects": [
+ {
+ "object": "S_USER_AGR",
+ "maintenanceStatus": "yFlag"
Can we have more meaningful values than "xFlag" or "yFlag"?
—
Reply to this email directly, view it on GitHub<#526 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AL74I5CJGKPNJ4VX2JEECQTXDEZSBANCNFSM6AAAAAAXK6FLMA>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
|
Validation "Validate JSON examples" fails because you changed only the example file. The corresponding ABAP type and the schema mention still "general". I will add two suggestions in a minute so that you can see which places I mean. Why "Validate JSON schema" fails is not clear to me, yet. I talked to @larshp yesterday. Our suggestion is: we finish the code review and if the check still fails, he helps us to check the issue and maybe fix it. |
There was a problem hiding this comment.
In general, it looks good to me.
I have just added some questions regarding $required and $showAlways which you might want to change.
Furthermore, I am wondering whether it is a good idea to rename the new introduced fields "description" to differentiate between the object's description.
See comments below for more details
file-formats/susi/examples/55fae0107acd2ee619d9f869c651edhs.susi.json
Outdated
Show resolved
Hide resolved
| "! Maintenance Status | ||
| "! $values {@link zif_aff_susi_v1.data:co_maintenance_status} | ||
| "! $default {@link zif_aff_susi_v1.data:co_maintenance_status.default_with_values} | ||
| "! $showAlways |
There was a problem hiding this comment.
Talked to @wurzka. This was changed by intention. I set this comment back to "resolved"
| "! Authorization fields. | ||
| "! <p class="shorttext">Description</p> | ||
| "! Description of authorization defaults of object | ||
| description TYPE string, |
There was a problem hiding this comment.
Just for consistency. If we decide to change it from "description" to something else, we should change it here, too.
There was a problem hiding this comment.
I am not sure what I need to do
There was a problem hiding this comment.
This comment is related to #526 (comment)
If we change the other location, I suggest to update both locations
No description provided.