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

[AIFI] Add new object type AIFI #647

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

Conversation

D052766
Copy link

@D052766 D052766 commented Aug 1, 2024

No description provided.

Copy link

cla-assistant bot commented Aug 1, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Aug 1, 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.

@Markus1812
Copy link
Member

Hi, thank you for submitting your AFF. Before we start with the review, please address the abaplint issues. They are mostly regarding unknown ABAP types, as you can only use standard ABAP types or pre-defined types from our types interface: zif_aff_types_v1.intf.abap.

For instance, /aif/proxy_outbound (char 30) can be replaced with ty_object_name_30 from zif_aff_types_v1.

Please also double-check that all titles are in title-case and descriptions follow the sentence-case scheme as described in our documentation.

"! <p class="shorttext">Integration Type</p>         <- title in title case
"! Integration type                                  <- description in sentence case
"! $required
integration_type TYPE ty_integration_type,

@albertmink albertmink added the new-object This is a new object type added to AFF label Aug 9, 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.

Commenting to change status to 'Changes Requested'

@D052766
Copy link
Author

D052766 commented Aug 20, 2024

Hi, thank you for submitting your AFF. Before we start with the review, please address the abaplint issues. They are mostly regarding unknown ABAP types, as you can only use standard ABAP types or pre-defined types from our types interface: zif_aff_types_v1.intf.abap.

For instance, /aif/proxy_outbound (char 30) can be replaced with ty_object_name_30 from zif_aff_types_v1.

Please also double-check that all titles are in title-case and descriptions follow the sentence-case scheme as described in our documentation.

"! <p class="shorttext">Integration Type</p>         <- title in title case
"! Integration type                                  <- description in sentence case
"! $required
integration_type TYPE ty_integration_type,

Hi, I have fixed now all issues that came up from the automated checks. Now that I cannot use the aif data types I think I need to add some enumerations. Also I think I need to double check whether all fields marked as required really need to be. Hope it is fine to have the pull request in place already so you can start the review!?

@D052766
Copy link
Author

D052766 commented Aug 26, 2024

I have added some enumerations, moved sap and raw structure to General Information and removed several "required" annotations. I think there should be no more changes until your first feedback now. Any chance of an estimation when that may be?

@D052766
Copy link
Author

D052766 commented Aug 28, 2024

Related to feedback I got from Katharina, I´ve just committed another update on the structure.

@Markus1812
Copy link
Member

Hi, thank you for updating the files. We'll perform a review tomorrow.

Copy link
Member

@Markus1812 Markus1812 left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the delay. In general, the AFF looks really good, but we have a few questions:

  1. Do you want to use prefixes like is_ or uses_ for booleans? We have marked some already and some already use prefixes. Maybe you can take a look again on that.
  2. Do you want your comments containing the name of your domains/database tables in a public repository?

file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
@Markus1812 Markus1812 added ux-review ready AFF is ready for UX review labels Sep 16, 2024
@Markus1812
Copy link
Member

Thank you for answering all our questions. The AFF looks good to me, you can now continue and request the ux-review when you are ready for that.

@D052766
Copy link
Author

D052766 commented Sep 19, 2024

I just removed the enumeration for the integration type. We have to supply this as custom value help from the backend. Please confirm the UX-REVIEW readiness is still given.

@Markus1812
Copy link
Member

@D052766 Yes sure, thats okay for me. Is there are reason behind this change? Maybe this would be interesting for the ux colleagues as well.

If your concern is the extensibility of the enum, our wiki states that adding values to the enum is compatible without increasing the format version, as long as there is a default value (which is the case).

@D047539
Copy link
Contributor

D047539 commented Sep 24, 2024

@Markus1812 We prefer to set and retrieve values in the backend, rather than defining them in the frontend.

@D052766
Copy link
Author

D052766 commented Oct 28, 2024

After AIF internal discussions and review I had to do some more adjustments. I think we´re ready for UX review now, but I will wait for your comments.

@Markus1812 Markus1812 removed the ux-review ready AFF is ready for UX review label Oct 28, 2024
@D052766
Copy link
Author

D052766 commented Oct 28, 2024

Key Field related wording has been reworked (I hope ;-))

@Markus1812
Copy link
Member

Thanks for the update. Ready for UX review 👌

@Markus1812 Markus1812 added the ux-review ready AFF is ready for UX review label Oct 29, 2024
Copy link
Contributor

@wurzka wurzka left a comment

Choose a reason for hiding this comment

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

Hi @D052766 ,
I went through the naming of the components and titles, please see my comments

file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
Copy link
Contributor

@wurzka wurzka left a comment

Choose a reason for hiding this comment

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

Thanks for the adaption of my comments. UX check revealed one more issue, see my comment

file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifi/type/zif_aff_aifi_v1.intf.abap Outdated Show resolved Hide resolved
Copy link
Contributor

@wurzka wurzka left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me.

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 ux-review ready AFF is ready for UX review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants