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

[EDCC] Add new object type for EDCC #661

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

Conversation

KUMARMUKULSAP
Copy link
Contributor

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, thank you for your aff. Definitely one of the biggest I have seen :)

{
"formatVersion": "1",
"header": {
"description": "AFF type for EDCK",

Choose a reason for hiding this comment

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

Suggested change
"description": "AFF type for EDCK",
"description": "AFF type for EDCC",

What does EDCC stand for?

message_type_description TYPE ty_short_description,
"! <p class="shorttext">Tax Authority Document Type</p>
"! Tax authority document type
taxauth_documenttype TYPE c LENGTH 20,

Choose a reason for hiding this comment

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

tax_authority_document_type

file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
original_language TYPE zif_aff_types_v1=>ty_original_language,
END OF ty_message,
"! <p class="shorttext">Tax Authority Message Types</p>
"! Tax Authority Message types

Choose a reason for hiding this comment

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

Suggested change
"! Tax Authority Message types
"! Tax authority message types

Sentence case

file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
@Markus1812 Markus1812 added the new-object This is a new object type added to AFF label Oct 1, 2024
@Markus1812 Markus1812 mentioned this pull request Oct 1, 2024
@KUMARMUKULSAP
Copy link
Contributor Author

@GuilhermeSaraiva96 : I have incorporated the above suggested changes. Please have a look and suggest if we can have a meeting to understand this AFF.

@KUMARMUKULSAP
Copy link
Contributor Author

@GuilhermeSaraiva96 : Let us know if the AFF requires further adjustment

file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edcc/type/zif_aff_edcc_v1.intf.abap Outdated Show resolved Hide resolved
@GuilhermeSaraiva96
Copy link

Hi, thank you for your changes, looks good already. I have just a few more suggestions. Additionally, could you please send a meeting request so that we can discuss the whole AFF before moving to the UX review? Thanks!

@KUMARMUKULSAP
Copy link
Contributor Author

@GuilhermeSaraiva96 : I have regenerated the examples and incorporated changes related to description of objects coming from other object types like EDCR/EDCK. Additionally I have added one new node in the AFF in ty_main, additional_selection_fields. Please have a look and let me know.

@GuilhermeSaraiva96
Copy link

From my side, it is ready for the UX-Review. @nicolas?
Note for the UX Colleagues: the country field is an attribute directly at the root level. Should it be wrapped into a 'General Information' structure? Then it will be shown in the selection Detail instead of as grid in the root level

@GuilhermeSaraiva96 GuilhermeSaraiva96 self-requested a review October 22, 2024 09:21
@KUMARMUKULSAP
Copy link
Contributor Author

@GuilhermeSaraiva96 : Above comments are fixed. please have a look

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.

Thanks! Looks good. Next time you can mark the comments as resolved, so that it is also easier to follow what has been changed, and what hasn't. Good job!

@GuilhermeSaraiva96
Copy link

ready for UX Review

@GuilhermeSaraiva96 GuilhermeSaraiva96 added ux-review ready AFF is ready for UX review and removed new-object This is a new object type added to AFF labels Oct 29, 2024
"! <p class="shorttext">Country</p>
"! Country
"! $required
country TYPE c LENGTH 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please put the country field into a structure general_information

Copy link
Contributor

@wurzka wurzka 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 my comments

"! <p class="shorttext">Country</p>
"! Country
"! $required
country TYPE c LENGTH 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this language field about? Are there any texts that must be translated?

Comment on lines +434 to +442
"! <p class="shorttext">Table Name</p>
"! Table name
table_name TYPE zif_aff_types_v1=>ty_object_name_30,
"! <p class="shorttext">Field Name</p>
"! Field name
field_name TYPE zif_aff_types_v1=>ty_object_name_30,
"! <p class="shorttext">Field Type</p>
"! Field type
field_type TYPE ty_field_type,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any mandatory field? With this setting, I could add an entry to the table that is completely empty having only fieldType: selectOption

Comment on lines +28 to +30
"! <p class="shorttext">Table Name</p>
"! Table name
table_name TYPE zif_aff_types_v1=>ty_object_name_30,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be required? Or can table_name be empty?

Comment on lines +272 to +275
"! <p class="shorttext">Comparison Type</p>
"! Comparison type
"! $required
comparison_type TYPE zif_aff_types_v1=>ty_object_name_30,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be enough:

Suggested change
"! <p class="shorttext">Comparison Type</p>
"! Comparison type
"! $required
comparison_type TYPE zif_aff_types_v1=>ty_object_name_30,
"! <p class="shorttext">Name</p>
"! Comparison type
"! $required
name TYPE zif_aff_types_v1=>ty_object_name_30,

Copy link
Contributor

Choose a reason for hiding this comment

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

Besides that, is this an ABAP object? Or is this something that is defined in the EDCC object.
I wondered whether the description of the comparison type is correctly stored here

Comment on lines +276 to +279
"! <p class="shorttext">Comparison Type Description</p>
"! Description of the comparison type
"! $required
description TYPE ty_long_description,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here:

Suggested change
"! <p class="shorttext">Comparison Type Description</p>
"! Description of the comparison type
"! $required
description TYPE ty_long_description,
"! <p class="shorttext">Description</p>
"! Description of the comparison type
"! $required
description TYPE ty_long_description,

Comment on lines +482 to +484
"! <p class="shorttext">eDocument Type Assignment</p>
"! Assign eDocument types associated with consistency scenario
edocument_types TYPE ty_edoc_types,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"! <p class="shorttext">eDocument Type Assignment</p>
"! Assign eDocument types associated with consistency scenario
edocument_types TYPE ty_edoc_types,
"! <p class="shorttext">eDocument Types</p>
"! Assign eDocument types associated with consistency scenario
edocument_types TYPE ty_edoc_types,

"! eDocument Type
edoc_type TYPE c LENGTH 10,
END OF ty_edoc_type.
"! <p class="shorttext">eDocument Type Assignment</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"! <p class="shorttext">eDocument Type Assignment</p>
"! <p class="shorttext">eDocument Types</p>

Comment on lines +389 to +391
"! <p class="shorttext">eDocument Type</p>
"! eDocument Type
edoc_type TYPE c LENGTH 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

Or only

Suggested change
"! <p class="shorttext">eDocument Type</p>
"! eDocument Type
edoc_type TYPE c LENGTH 10,
"! <p class="shorttext">Type</p>
"! eDocument Type
type TYPE c LENGTH 10,

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a mandatory field?

Comment on lines +412 to +414
"! <p class="shorttext">Inconsistency Category</p>
"! Inconsistency category
result_ui_group TYPE ty_resultgroup,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"! <p class="shorttext">Inconsistency Category</p>
"! Inconsistency category
result_ui_group TYPE ty_resultgroup,
"! <p class="shorttext">Category</p>
"! Inconsistency category
category TYPE ty_resultgroup,

country_xtension TYPE zif_aff_types_v1=>ty_object_name_30,
"! <p class="shorttext">Result Processes</p>
"! Assign result process to the UI group
result_process TYPE ty_result_processes,
Copy link
Contributor

Choose a reason for hiding this comment

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

These result processes have been there before (field of checks). Do we need them twice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ux-review ready AFF is ready for UX review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants