-
Notifications
You must be signed in to change notification settings - Fork 56
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
[AIFR] - Adding field aifRecipient to AFF for AIFR #673
base: main
Are you sure you want to change the base?
Conversation
Technically, adding new mandatory fields is considered as an incompatible change. However, since AIFR is still in development, I think we can still accept incompatible changes. This means, we need to ignore the failing compatibility check in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the addition looks good to me. I have just added a comment wrt "AIF" prefix. Maybe, others have an opinion on this.
@@ -6,6 +6,7 @@ | |||
"abapLanguageVersion": "cloudDevelopment" | |||
}, | |||
"generalInformation":{ | |||
"namespace": "AIFNS" | |||
"namespace": "AIFNS", | |||
"aifRecipient": "REC_WS_INB" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix "AIF" has also been used in other places (like for AIFN for the namespace):
"aifNamespace": "/AIFT" |
Therefore, I guess it is fine here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We refer to the development object: Namespace (AIFN). The namespace (AIFN) contains the field AIF namespace, in the AIFN object, we convert the name of the namespace (AIFN) into our own AIF namespace. This is necessary due to historical reasons.
In this case it is the namespace and not the internal AIF namespace which needs to be assigned to the recipient, so the prefix would not be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Sorry, my comment was misleading.
My comment should have been related to field aifRecipient
(not to field namespace
). I just wanted to state, that it is most probably fine to use "aif" as prefix for this field as you did it for aifNamespace
in AIFN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. AIF is often used in the editors of this context. Therefore, it is fine for me here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We performed UX check on this change. Looks good to me.
Short explanation of the background:
During our tests, we discovered the need for an additional field in the AIFR, specifically the AIF Recipient.
It must be possible to assign the same AIF recipient to different namespaces.
The object name is derived from the namespace and the AIF Recipient. To ensure accurate derivation, we include both fields in the creation wizard and set the object name as read-only.
Since the exact derivation from object name Recipient back to Namespace and AIF Recipient may not always be possible, we displayed both fields as read-only in the editor for informational purposes.