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

[INTM] Add new object type INTM #671

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

Conversation

raghav6686
Copy link

No description provided.

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.

Hi, thanks for this AFF as well. I had reviewed the INTS aff, and the comments are similar to both. So if you apply some of the changes, please check that the same actions (sentence case, capitalised words in titles) are applied for both AFFs

file-formats/intm/type/zif_aff_intm_v1.intf.json Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
@schneidermic0 schneidermic0 added the new-object This is a new object type added to AFF label Oct 29, 2024
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.

See my first iteration of questions below

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

Hi, thanks for this AFF as well. I had reviewed the INTS aff, and the comments are similar to both. So if you apply some of the changes, please check that the same actions (sentence case, capitalised words in titles) are applied for both AFFs

Done the changes. Please review and let me know in case of any changes required.

file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intm/type/zif_aff_intm_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.

Please see below some more general questions/remarks based on your example object.

Some of them we addressed in our meeting last week (like the GUID and the "Published" property). Some might be new. Structure-wise I think the prompt parameters are worth to look into. Maybe, we can get rid of the JSON content field by replacing it in a well structured information that is much more readable, but I am not sure, whether this is technically possible.

@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.

@raghav6686 Just a minor comment. I think this can be fixed (if it helps you), later. But before merging this PR.

I will also reopen one older comment and provide you a suggestion. To make it more clear to you what I meant. With the prompts. This can also be fixed next week.

"promptTemplateName": "USER_PROMPT",
"promptTemplateDescription": "User prompt",
"prompt": "Build a confirmation mail for a sales order containing {ISLM_Items} where the items are categorized for example pencil and paper will be stationary, printer and mouse will be electronics. The source address contains {ISLM_Source} and the delivery address is in {ISLM_Destination}.",
"promptParameters": "[{\"Name\":\"ISLM_Items\",\"Value\":\"\"},{\"Name\":\"ISLM_Source\",\"Value\":\"\"},{\"Name\":\"ISLM_Destination\",\"Value\":\"\"}]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to #671 (comment). If it is always name-value or an empty list , it could look like this in AFF:

Suggested change
"promptParameters": "[{\"Name\":\"ISLM_Items\",\"Value\":\"\"},{\"Name\":\"ISLM_Source\",\"Value\":\"\"},{\"Name\":\"ISLM_Destination\",\"Value\":\"\"}]",
"promptParameters": [
{ "Name": "ISLM_Items",
"Value": ""
},
{ "Name": "ISLM_Source",
"Value ": ""
},
{ "Name ": "ISLM_Destination",
"Value": ""
}
]",

Copy link
Contributor

Choose a reason for hiding this comment

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

Can be discussed next week

Copy link

@raghav6676 raghav6676 Nov 22, 2024

Choose a reason for hiding this comment

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

Actually, our logic in AFF persistence class only builds the JSON and pass to string field, which is necessary for passing the parameters in the prompt execution.
image

image

Does AFF/ADT framework stringify the JSON in the source? I mean the / in the output.

Also another comment on this VALUE field - This is empty as of now, but as I mentioned earlier, we will definitely implement the default values option as user input in the ADT UI. Similar to our Fiori UI,
image
So, this default parameters is future scope for ADT.

Copy link
Contributor

@schneidermic0 schneidermic0 Nov 22, 2024

Choose a reason for hiding this comment

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

Does AFF/ADT framework stringify the JSON in the source? I mean the / in the output.

Since you put JSON in a string field consisting of certain characters (to be precise: quotes (")), these quotes must be escaped by \.

Nevertheless, even without escaping, it would be nicer if you don't put the JSON into a string, but just using it as real JSON. Still, I am not sure whether this is possible in your case, but maybe it's worth to discuss.

Did you get my idea?

Choose a reason for hiding this comment

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

Yes, I got your point that you want to keep it as real JSON. We kept it as string because we do store the prompt parameters in DB as a string.
If we keep it as real JSON, then we need to serialize it and store in the DB.
Also, AFF has to be changed for prompt_parameters field, which needs to be declared as a deep table within prompts table. Is it possible?

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. :(

Yes, I think you would need to serialize it, if you would change it. I am still not 100% sure whether it is feasible at all to follow my suggested approach in your case. But if it is feasible, I would recommend it.

Maybe, it is worth to discuss my idea in a call. Then we can see the advantages and disadvantages of both approaches. What do you think?

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