Skip to content
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

[RVBC] Add new object type RVBC #677

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

RuixingYangSAP
Copy link

No description provided.

Copy link

cla-assistant bot commented Dec 5, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Dec 5, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@huber-nicolas huber-nicolas added the new-object This is a new object type added to AFF label Dec 6, 2024
Copy link

@GuilhermeSaraiva96 GuilhermeSaraiva96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your AFF! Here are my suggestions

file-formats/rvbc/type/zif_aff_rvbc_v1.intf.json Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
@RuixingYangSAP
Copy link
Author

Hi @GuilhermeSaraiva96, Thank you for taking time reviewing our AFF. I have changed the AFF according to the suggestions from the review. Could you please kindly help to have a check again? Best regards, Ray

@RuixingYangSAP
Copy link
Author

Hi @GuilhermeSaraiva96 , I have changed the AFF and the description header in the JSON file according to our call yesterday. In addition I added the ID field which is the review booklet ID, which for the similar reason of ina1_service_id it needs to be carried all the way to be able to run RAP based EML statement to identify the correct review booklet and do the corresponding changes.
please kindly help to have another review to see if everything is ok.

file-formats/rvbc/type/zif_aff_rvbc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/rvbc/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @RuixingYangSAP . I think you made great improvements.

I have added some further comments/questions.

"bookletDefinition": {
"application": "test.app.adt.editor",
"source": "predefined",
"status": "draft",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in our call, I am wondering whether the draft is the same as active/inactive handling in ABAP

"application": "test.app.adt.editor",
"source": "predefined",
"status": "draft",
"consistencyStatus": "consistent",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar question here:
Usually, in ABAP tools, consistency is checked using syntax-check or (maybe) ATC checks. It is nothing which is stored in the object data. Could I just change it to "inconsistent" in the editor ;)


File | Cardinality | Definition | Schema | Example
:--- | :--- | :--- | :--- | :---
`<name>.rvbc.json` | 1 | [`zif_aff_rvbc_v1.intf.abap`](./type/zif_aff_rvbc_v1.intf.abap) | [`rvbc-v1.json`](./rvbc-v1.json)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to the example file is missing

Comment on lines +93 to +96
"! <p class="shorttext">Service Name</p>
"! Service name
"! $required
ina1_service TYPE ty_ina1_service_name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, we try to keep the name of the tile and the field name identical. So either "Service Name" or "INA1 Service".

Comment on lines +24 to +36
"! $values { @link zif_aff_rvbc_v1.data:co_source }
"! $default { @link zif_aff_rvbc_v1.data:co_source.predefined }
TYPES ty_source TYPE n LENGTH 1.
CONSTANTS:
BEGIN OF co_source,
"! <p class="shorttext">Predefined</p>
"! Predefined
predefined TYPE ty_source VALUE 1,
"! <p class="shorttext">Custom</p>
"! Custom
custom TYPE ty_source VALUE 2,
END OF co_source.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the type and the constant is obsolete, now, isn't it?

PUBLIC.

TYPES ty_app_id TYPE c LENGTH 70.
TYPES ty_ina1_service_name TYPE c LENGTH 40.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. Is the service name really 40 characters long. Most objects only support 30 characters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-object This is a new object type added to AFF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants