Skip to content

[AIFA] Add new object type #703

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

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

Conversation

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

Thank you for your AFF! I have a few questions and some minor comments :)

BEGIN OF ty_fields_restore,
"! <p class="shorttext">Field Name</p>
"! Field name
field_name TYPE c LENGTH 255,

Choose a reason for hiding this comment

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

Suggested change
field_name TYPE c LENGTH 255,
name TYPE c LENGTH 255,

BEGIN OF ty_fields_restore,
"! <p class="shorttext">Field Name</p>
"! Field name
field_name TYPE c LENGTH 255,

Choose a reason for hiding this comment

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

Is this length correct? 255

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its string now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IN the future it will be field path. In case we have a deep structure with many substructures the path will be longer than char 40.

"! <p class="shorttext">AIF Action</p>
"! AIF action
"! $required
aif_action TYPE c LENGTH 20,

Choose a reason for hiding this comment

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

Is the aif_action the name of the object instance itself?

"! <p class="shorttext">Check</p>
"! Check
"! $required
check_obj_name TYPE zif_aff_types_v1=>ty_object_name_30,

Choose a reason for hiding this comment

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

Suggested change
check_obj_name TYPE zif_aff_types_v1=>ty_object_name_30,
check TYPE zif_aff_types_v1=>ty_object_name_30,

It is implicit that refers to the name of the check object

Comment on lines +63 to +64
"! <p class="shorttext">Field Name</p>
"! Field name

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">Field Name</p>
"! Field name
"! <p class="shorttext">Fields</p>
"! Fields

Comment on lines +67 to +68
"! <p class="shorttext">Check Data</p>
"! Check data

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">Check Data</p>
"! Check data
"! <p class="shorttext">Checks</p>
"! Checks

field_restore TYPE ty_fields_to_restore,
"! <p class="shorttext">Checks</p>
"! Checks
check_data TYPE ty_checks,

Choose a reason for hiding this comment

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

Suggested change
check_data TYPE ty_checks,
checks TYPE ty_checks,

general_information TYPE ty_general_information,
"! <p class="shorttext">Fields To Restore</p>
"! Fields to restore
field_restore TYPE ty_fields_to_restore,

Choose a reason for hiding this comment

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

Suggested change
field_restore TYPE ty_fields_to_restore,
fields_to_restore TYPE ty_fields_to_restore,

general_information TYPE ty_general_information,
"! <p class="shorttext">Fields To Restore</p>
"! Fields to restore
field_restore TYPE ty_fields_to_restore,

Choose a reason for hiding this comment

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

What are these fields to restore? How do they fit into the workflow of the AIF Action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes, later action classes depend on information calculated by earlier action classes. If a later action class fails and the system restarts, an earlier action class might not run again, causing the needed information to be lost. To prevent errors in the restarted process, a "field to restore" can be used. This field saves the important information, so if an action class needs to be re-run after an error, it can retrieve the saved information and continue correctly.

"! <p class="shorttext">Number</p>
"! Number
"! $required
number TYPE n LENGTH 3,

Choose a reason for hiding this comment

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

The table is of type standard, which means that the order of the items in the table define a sequence, which can be used to derive this 'number'. Basically, the first item in the list would be the first check to be performed. In this sense, the 'number' field here be useless. This also makes it more intuitive to use in our Editors. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I discussed with colleagues, and we agreed that we can remove the number field from the UI.

Choose a reason for hiding this comment

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

Can't another field be the key? For example, the check name. Are you expecting multiple entries with the same check name?

From my point of view, I cannot see the advantage in you keeping this numeric_id, as with the table type standard, there is already an implied id in each data entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, actually we do expect multiple entries with the same check name. We can do same check for few fields. And Field may be longer than 128, because its field path actually. So, 'field path' can't be the key as well. We need number as a key.

@Smartynkov
Copy link
Contributor Author

Hi, we had a long discussion with a team about changes with Action file format and here is the summary:

We need number id because it's a key field for our table and its shouldn't be calculated automatically. Earlier it was a key and an order number of a check, we decided to spilt t up and now number field isn't about an order of a check and just a key field. we have a new field for the order, and it will be defined by the order of a check in the table on the UI we will add order changeable to the table for that to happen.

Also, there was some discussion about 'number' as a name for this key field on the UI and we think it's not showing what is this field is about. We propose name 'numeric_id', but we are also opened for the discussion.

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.

2 participants