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

[SMTG] Add new object type SMTG #633

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

Conversation

OliverBeck01
Copy link

No description provided.

Copy link

cla-assistant bot commented Jul 10, 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.

1 similar comment
Copy link

cla-assistant bot commented Jul 10, 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.

@GuilhermeSaraiva96
Copy link

missing an example as well

@albertmink albertmink added the new-object This is a new object type added to AFF label Jul 10, 2024
@huber-nicolas huber-nicolas self-assigned this Jul 11, 2024
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 found some minor findings (see my comments). But I think that the AFF structure needs to be changed nearly completely. Maybe we can have a call and talk about it.

file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zob_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
@huber-nicolas
Copy link
Contributor

Please expect a delay for the review of this pull request. We have to clarify the language dependent fields of this object.

@huber-nicolas
Copy link
Contributor

huber-nicolas commented Aug 6, 2024

Thank you for submitting your object type to the AFF Repository. After detailed discussion with @wurzka @schneidermic0 please go over my comments above and solve them. Please also solve the abaplint issues.

As we discussed in a previous meeting, please remove the ty_template_content, in order to put the language parts into separate files.

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.

Thanks for updating the AFF. I've added some new comments with some optional suggestions that are up to you to decide.
If you have any questions regarding my comments, feel free to reach out.

file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/smtg/type/zif_aff_smtg_v1.intf.abap Outdated Show resolved Hide resolved
@OliverBeck01
Copy link
Author

Thank you for the feedback. I changed the requested lines and uploaded the updated documents.

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, thank you again for adapting our suggestions. One more question from our side:

Edit: Please also sign the Contributor License Agreement as described in #633 (comment) to make all automated checks happy.

cds_view TYPE zif_aff_types_v1=>ty_object_name_30,
"! <p class="shorttext">Subject</p>
"! The subject of the email template
email_subject TYPE c LENGTH 1024,
Copy link
Member

Choose a reason for hiding this comment

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

Should the email subject be required?

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants