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 10 commits into
base: main
Choose a base branch
from
Open

[EDOT] Add new object EDOT #682

wants to merge 10 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)

Comment on lines 56 to 59
"! <p class="shorttext">eDocument Type</p>
"! eDoc Type
"! $required
edocument_type TYPE ty_edoc_type,
Copy link
Member

Choose a reason for hiding this comment

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

What values can edocument_type have? Is this a fixed set of values?

Copy link
Author

Choose a reason for hiding this comment

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

This is the main object name, created by Developer during design time with new names

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is the actual object's name also stored in TADIR, right?

If this is the case, please remove it. The object name is stored in the filename like "my_object_name.edot.json"

Copy link
Author

Choose a reason for hiding this comment

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

corrected; removed object name and description from general information

Comment on lines 60 to 63
"! <p class="shorttext">Description</p>
"! eDocument Type Description
"! $required
edoc_type_desc TYPE ty_short_description,
Copy link
Member

Choose a reason for hiding this comment

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

Same question as in EDOI, is this the description of the EDOT object? I see this is only 30 characters, but how does this differ from the description that is contained in ty_main->header?

Copy link
Author

Choose a reason for hiding this comment

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

Do we need to use ty_main->header for description or we can create new field in the structure?

The description can be max of 30 characters as designed by Database table when it was created

Copy link
Member

Choose a reason for hiding this comment

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

The fields for name, description, and original language (as seen in the creation wizard) should not be defined by you in the AFF.
The name of an object (that is entered in the creation wizard and used to uniquely identify your object) is saved in the filename on deserialization. When looking at the server-driven editor, the name is shown in the title section and the repository browser.
The fields description and original language are persisted in the ty_main->header structure, that every AFF needs to have. This means, that you do not have to define these fields yourself.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, one small addition to Markus' comment. Even if I think 30 character description is very short, it is technically possible to define a header with a character-30-field for the description.

In this case the header must be defined within the object type in a similar way as it is done here:

TYPES:
"! <p class="shorttext">Header for SAJC Object</p>
"! The header for an application job catalog entry
BEGIN OF ty_header,
"! <p class="shorttext">Description</p>
"! Description of the application job catalog entry
"! $required
description TYPE c LENGTH 120,
"! <p class="shorttext">Original Language</p>
"! Original language of the application job catalog entry
"! $required
original_language TYPE sy-langu,
"! <p class="shorttext">ABAP Language Version</p>
"! ABAP language version
"! $values {@link zif_aff_types_v1.data:co_abap_language_version_cloud}
"! $default {@link zif_aff_types_v1.data:co_abap_language_version_cloud.standard}
abap_language_version TYPE zif_aff_types_v1=>ty_abap_language_version_cloud,
END OF ty_header.

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 details. I created new header with 30char length description.

Comment on lines 67 to 69
"! <p class="shorttext">eDocument Type Created Using Contingency</p>
"! Contingency Type
contingency_type TYPE c LENGTH 10,
Copy link
Member

Choose a reason for hiding this comment

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

Does this have a set of fixed values?

Copy link
Author

Choose a reason for hiding this comment

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

No, this will be another eDocument Type value which is created before

file-formats/edot/type/zif_aff_edot_v1.intf.abap Outdated Show resolved Hide resolved
Comment on lines 16 to 29
"! <p class="shorttext">File Type</p>
"! File Type
"! $required
file_type TYPE c LENGTH 10,
"! <p class="shorttext">File Structure Type</p>
"! File Structure type
"! $required
file_structure_type TYPE c LENGTH 30,
"! <p class="shorttext">File Description</p>
"! File Description type
file_description_type TYPE c LENGTH 60,
"! <p class="shorttext">File Cloud Relevancy</p>
"! File Cloud Relevancy type
not_cloud_relevant_type TYPE abap_bool,
Copy link
Member

Choose a reason for hiding this comment

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

Why are all types ending with _type? Isn't this more like file_type, file_structure, file_description, and not_cloud_relevant?

  • file_type: Does this have a fixed set of values? Do you have examples for possible file types?
  • file_structure: Same question, do you have example values?
  • not_cloud_relevant: Wouldn't it be easier to understand if you flip the bool: is_cloud_relevant? The title is also File Cloud Relevancy.

Copy link
Author

Choose a reason for hiding this comment

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

file_type: Its list of entries maintained in other table EDOFILETYPE but not fixed as entries can be added in Value table. Example INBOUND, REQUEST, RESPONSE, HTML_PREVIEW etc.

file_structure: It represent the ABAP structures to be used to display the file. so that we can enable the read access log when data viewed by user.

changed from not_cloud_relevant to is_cloud_relevant

Copy link
Contributor

Choose a reason for hiding this comment

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

file_type: Its list of entries maintained in other table EDOFILETYPE but not fixed as entries can be added in Value table. Example INBOUND, REQUEST, RESPONSE, HTML_PREVIEW etc.

General remark for fixed values/enum values: For me, the question is not how this is technically solved (hard coded values, domain, value table, ...) but who maintain the possible values?

If you (as object type owner) maintain the values, I think an enum will bring benefits for UX and validation of the values.

If the values are maintained using extension mechanisms (e.g., consumers of your object type can extend the set values on their own), I agree, enums don't make sense at all.

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

Comment on lines 27 to 29
"! <p class="shorttext">File Cloud Relevancy</p>
"! File Cloud Relevancy type
not_cloud_relevant_type TYPE abap_bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see for a lot of fields a mismatch in the title ("File Cloud Relevancy"; and its description) and the field name (not_cloud_relevant_type). It would be great if this can be harmonized in general.

However, in this case it seems to me just wrong:

File Cloud Relevancy = "Yes" vs. not_cloud_relevant_type = true

Copy link
Author

Choose a reason for hiding this comment

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

Corrected

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned by @Markus1812, all EDOI files should not be part of this PR

Copy link
Author

Choose a reason for hiding this comment

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

EDOI files removed

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

"! <p class="shorttext">Header</p>
"! Header
"! $required
header TYPE zif_aff_types_v1=>ty_header_60,
Copy link
Contributor

Choose a reason for hiding this comment

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

What ABAP language versions does your object type support? "standard", "keyUser", "cloudDevelopment"

Copy link
Author

Choose a reason for hiding this comment

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

Standard & cloudDevelopment

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.

Comment on lines 56 to 59
"! <p class="shorttext">eDocument Type</p>
"! eDoc Type
"! $required
edocument_type TYPE ty_edoc_type,
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is the actual object's name also stored in TADIR, right?

If this is the case, please remove it. The object name is stored in the filename like "my_object_name.edot.json"

@i506210 i506210 requested a review from schneidermic0 February 6, 2025 09:35
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.

3 participants