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

[INTS] Add new object type INTS #670

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

Conversation

raghav6686
Copy link

No description provided.

Copy link

cla-assistant bot commented Oct 29, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Oct 29, 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.

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 submitting the AFF. I have added a few comments. Since it is a big AFF and it is important that we understand how it works, would you be open for a meeting?

file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
@huber-nicolas huber-nicolas self-requested a review October 29, 2024 13:19
@schneidermic0 schneidermic0 added the new-object This is a new object type added to AFF label Oct 29, 2024
@GuilhermeSaraiva96
Copy link

We have spoken within the team, could you please setup a meeting with me (Guilherme Saraiva), Nicolas Huber and Michael Schneider? We do not usually ask for meeting, but due to the urgency of the delivery and the complexity of the AFF, we think it is best.

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.

Sentence case means that only the first letter of the first word is capitalised.
Sorry that you changed all of the descriptions, they need to be reverted

file-formats/ints/ints-v1.json Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap 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.

I have added several comments now. In a first step, I would focus on structural changes of the AFF like: bindings and binding_mappings or signature*, modeling* and parameter*.

Furthermore, an example would be nice to see the references of INTM to INTS.

I expect more feedback after this iteration.

file-formats/ints/README.md Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
@raghav6686
Copy link
Author

I have added several comments now. In a first step, I would focus on structural changes of the AFF like: bindings and binding_mappings or signature*, modeling* and parameter*.

Furthermore, an example would be nice to see the references of INTM to INTS.

I expect more feedback after this iteration.

I've done the changes. Please review and let me know in case of any comments.

@raghav6686
Copy link
Author

Sentence case means that only the first letter of the first word is capitalised. Sorry that you changed all of the descriptions, they need to be reverted

I've done then changes.

file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
Copy link
Contributor

@huber-nicolas huber-nicolas left a comment

Choose a reason for hiding this comment

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

I only made some comments regarding nameing of fields and titles

file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
@schneidermic0
Copy link
Contributor

I had a look at recent changes. But I haven't managed to look into all details, yet. This object has a huge list of fields.

@huber-nicolas gave some input on differences between the title (shorttext description) and the field name. If possible we should keep them in sync. However, whether you change the field name or the title is up to you. I guess, there we see anyhow further suggestions during UX review. But I would keep them in sync already.

@huber-nicolas huber-nicolas added the ux-review ready AFF is ready for UX review label Nov 5, 2024
@albertmink albertmink requested a review from a team November 6, 2024 15:22
Copy link
Contributor

@albertmink albertmink left a comment

Choose a reason for hiding this comment

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

Thanks @raghav6686. LGTM in my role as UX person

Copy link
Contributor

@huber-nicolas huber-nicolas left a comment

Choose a reason for hiding this comment

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

Looks good to me

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.

Two more small requests. But no crucial changes from my point of view.

I didn't went through completely, yet.

file-formats/ints/type/zif_aff_ints_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/ints/examples/rm_test_adt_976.ints.json Outdated Show resolved Hide resolved
"state": "Published",
"islmVersion": 1.0
},
"classInformation": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is classInformation required information? If so, I would expect content within the example.

Copy link
Author

Choose a reason for hiding this comment

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

As we discussed earlier, for 2502 we are keeping only few fields for ADT UI and many fields we are hidings and some need to be filled in Original system only. So, this class information, we are hiding as of now, but definitely it's important for us and we'll unhide it in further release. Actually, we got UX review comment to rename the section as General information. Then, we had internal discussion within our team and decided to divide the section similar to our Fiori UI.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see. But in this case field classInformation must not be a required structure as it is stated right now.

This is not critical issue. It can be fixed somewhen next week. But for sure you can also fix it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, don't resolve a conversations. This makes it hard for us to see which questions we need to follow up or what we should double-check after changes. It is sufficient to add your answer.

Choose a reason for hiding this comment

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

As I mentioned earlier, we're keeping all the fields in the interface and hiding many fields which are not part of 2502. So this class information is for next scope.

But I'm not sure why it's giving empty set in object source JSON. May be because this is a structure?
Actually, I've hidden other tables which are empty from the get_content method and they're not part of the object source JSON. Also, I'm not passing the class information in get_content.

May I know why only empty structure is displayed in the object source JSON and not empty tables?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't manage to answer earlier. :(

I think the structure is rendered by default, because it is marked with the $required annotation.

"! $required
class_information TYPE ty_class_information,

If I understood your last comment, the classInformation must not be set in any INTS object. Therefore, you could/should remove the $required annotation.

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.

6 participants