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

[EDOT] Add new object EDOT #682

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

[EDOT] Add new object EDOT #682

wants to merge 13 commits into from

Conversation

i506210
Copy link

@i506210 i506210 commented Jan 28, 2025

No description provided.

@i506210 i506210 changed the title [EDOTFeature/edot [EDOT] Add new object EDOT Jan 28, 2025
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 your pull request. This pull request still contains both, EDOI and EDOT (with the .DS_Store). Please remove all the unwanted files here.

Please note, that for the title and description, we use title case and sentence case respectively. I've not marked every occurrence but if you could please adjust that.

"! <p class="shorttext">Interface Version</p>    "<- title (title case)
"! Interface version                             "<- description (sentence case)

file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edot/type/zif_aff_edot_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 also added some comments and questions

file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edoi/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be deleted

Copy link
Author

Choose a reason for hiding this comment

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

somehow i coudn't see this hidden file but anyhow i replaced whole folder

Copy link
Member

Choose a reason for hiding this comment

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

The file is there again, which is strange as it is excluded in our .gitignore. Assuming you are on macOS, can you please try the steps described here on your feature branch? https://www.geeksforgeeks.org/how-to-remove-ds_store-files-from-git-repositories/

Copy link
Author

Choose a reason for hiding this comment

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

thank you for the link, I have deleted and push to git. please check now

Copy link
Contributor

Choose a reason for hiding this comment

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

Example is missing

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having an example would also help me for the review

Copy link
Author

Choose a reason for hiding this comment

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

Somehow I am unable to generate example, I created class and Transformation but its not clear to me in the report what object to be mentioned as input.

file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
Comment on lines 77 to 82
"! <p class="shorttext">Additional Selection Fields</p>
"! Additional selection fields of validation report
edocument_sral_configuration TYPE ty_sral_configurations,
"! <p class="shorttext">eDocument Type Specific Additional Tables</p>
"! eDocument Type Specific Additional Tables
edoc_spec_additional_table TYPE ty_additional_tables,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering whether it make sense to move these two fields (representing tables/arrays) on top level into ty_main. Maybe, you want to check this also during UX review.

For "general information" it seems to me too detailed.

Copy link
Author

Choose a reason for hiding this comment

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

Moved to main block

file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
@i506210 i506210 requested a review from schneidermic0 February 6, 2025 09:35
@i506210
Copy link
Author

i506210 commented Feb 11, 2025

I have also added some comments and questions

I corrected and pushed the latest changes, can you please check

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 and changes. Most of my comments are minors and just a personal touch, up to you if you want to implement them :)
However, I always like to mention that AFF is a readable representation of an ABAP Object Type, and the more coherent the field names are, the better!

contingency_type TYPE c LENGTH 10,
"! <p class="shorttext">Preprocess Before Archiving</p>
"! Preprocess before archiving
archive_prep_req TYPE abap_bool,

Choose a reason for hiding this comment

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

Suggested change
archive_prep_req TYPE abap_bool,
archive_preprocess TYPE abap_bool,

Copy link
Author

Choose a reason for hiding this comment

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

corrected

file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
table_name TYPE zif_aff_types_v1=>ty_object_name_30,
"! <p class="shorttext">Created in Contingency Mode</p>
"! Created in contingency mode
contingency_mode TYPE abap_bool,

Choose a reason for hiding this comment

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

What is the contingency_mode referring to? to the edoc type or to the table?
Not really familiar with the topic, but it seems like it could belong to the table. Am I correct?
I would also like to understand the relationship between the contingency_mode and the contingency_edoc_type. Could it be that contingency_edoc_type is only relevant when contingency_mode is true?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, contingency_edoc_type is relevant when contingency mode is activated.
Contingency is a process related to eInvoice solution that introduced by Government authorities (some countries) to process Invoice in offline instead of online submission when Tax authority server is down.

In our UI, user switch the invoice to contingency mode.

Choose a reason for hiding this comment

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

As discussed in the call, contingency_mode and the attribute below are moved to the bottom of the structure

"! Created in contingency mode
contingency_mode TYPE abap_bool,
"! <p class="shorttext">eDocument Type Created Using Contingency</p>
"! eDocument type created using contingency

Choose a reason for hiding this comment

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

Suggested change
"! eDocument type created using contingency
"! Contingency eDocument type

same for the shortest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants