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

Merged
merged 42 commits into from
Nov 14, 2024
Merged

[AIFI] Add new object type AIFI #647

merged 42 commits into from
Nov 14, 2024

Conversation

D052766
Copy link
Contributor

@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
@GuilhermeSaraiva96 GuilhermeSaraiva96 self-requested a review August 19, 2024 13:19
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
Contributor 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
Contributor 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
Contributor 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?

@D052766
Copy link
Contributor 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
Contributor 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
Contributor 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

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

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.

@wurzka wurzka requested review from GuilhermeSaraiva96 and removed request for GuilhermeSaraiva96 November 13, 2024 14:13
@wurzka wurzka merged commit 942e346 into SAP:main Nov 14, 2024
10 checks passed
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