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

feat(RuleTicket): add new action 'replace' for actors #15718

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

Conversation

stonebuzz
Copy link
Contributor

@stonebuzz stonebuzz commented Oct 5, 2023

add new action 'replace' for actors

🤖 Generated by Copilot at 236c2b4

This pull request adds a feature to replace actors of ITIL objects based on rules, instead of appending them. It modifies the CommonITILObject, RuleCommonITILObject, and RuleAction classes to handle the new action type and input keys for replacing actors. It also adds a test function to verify the feature in tests/functional/RuleTicket.php.

🤖 Generated by Copilot at 236c2b4

  • Add a new variable to store the input key for replacing actors of a given itemtype and type in the CommonITILObject class (link)
    • Unset the input key for replacing actors from the input array (link)
  • Add a new option replace to the getActions function in the RuleAction class. The option replace is used to indicate that the action type is to replace the value of a field by another value, instead of assigning or appending it (link)
    • Otherwise, call the parent function to execute the action as usual (link)
    • _users_id_requester: replace users as requesters, with the input key _replace_users_requesters (link)
    • _groups_id_requester: replace groups as requesters, with the input key _replace_groups_requesters (link)
    • _users_id_assign: replace users as assigns, with the input key _replace_users_assigns (link)
    • _groups_id_assign: replace groups as assigns, with the input key _replace_groups_assigns (link)
    • _suppliers_id_assign: replace suppliers as assigns, with the input key _replace_suppliers_assigns (link)
    • _users_id_observer: replace users as observers, with the input key _replace_users_observers (link)
    • _groups_id_observer: replace groups as observers, with the input key _replace_groups_observers (link)
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets !29783

@stonebuzz stonebuzz self-assigned this Oct 5, 2023
@stonebuzz stonebuzz added the bug label Oct 5, 2023
@stonebuzz stonebuzz added this to the 10.0.11 milestone Oct 5, 2023
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

I am not against this change, but last time we discussed about it, we said that we should not do it in a bugfixes version.

see #11957 (comment)

@stonebuzz
Copy link
Contributor Author

as you like (I can rebase on main) but it's a bug that's been hanging around until now

the rule engine offers 'assign' and 'append' action, the two should have a different behaviour

@orthagh what do you think?

@cedric-anne
Copy link
Member

cedric-anne commented Oct 9, 2023

the rule engine offers 'assign' and 'append' action, the two should have a different behaviour

In fact, they act differently, but maybe not as people can expect it.

  1. Create a ticket with A as assignee.
  2. Update the ticket to assigne B and C.
    -> A assign X rule will remove the new assignees B and C and assign X. Result will be to have A and X as assignees.
    -> A append X rull will append X to B and C. Result will be to have A, B, C and X as assignees.

@orthagh what do you think?

We talk about it just now. He said it is preferable to fix it in main branch.

@cedric-anne cedric-anne changed the base branch from 10.0/bugfixes to main October 9, 2023 09:03
@cedric-anne cedric-anne modified the milestones: 10.0.11, 10.1.0 Oct 9, 2023
@stonebuzz
Copy link
Contributor Author

A assign X rule will remove the new assignees B and C and assign X. Result will be to have A and X as assignees.

No, for me :

A assign X rule will remove old assign A the new assignees B and C and assign X. Result will be to have only X as assigne.

@cedric-anne
Copy link
Member

A assign X rule will remove the new assignees B and C and assign X. Result will be to have A and X as assignees.

No, for me :

A assign X rule will remove old assign A the new assignees B and C and assign X. Result will be to have only X as assigne.

I was explaining the current result. What you describe is the result of your proposal.

@stonebuzz stonebuzz force-pushed the fix_rule_assign_group branch 3 times, most recently from bb8ef70 to 2c698b0 Compare October 13, 2023 06:01
@stonebuzz stonebuzz changed the title fix(Ruleticket): fix assign action for group feat(Ruleticket): add new action 'replace' for actors Oct 17, 2023
@stonebuzz stonebuzz changed the title feat(Ruleticket): add new action 'replace' for actors feat(RuleTicket): add new action 'replace' for actors Oct 17, 2023
src/CommonITILObject.php Outdated Show resolved Hide resolved
src/CommonITILObject.php Outdated Show resolved Hide resolved
@@ -1167,4 +1167,118 @@ public function testAssignLocationFromUser(
$ticket->getFromDB($ticket->getID());
$this->integer($ticket->fields['locations_id'])->isEqualTo($expected_location_after_creation);
}

public function testAssignGroupOnUpdate()
Copy link
Member

Choose a reason for hiding this comment

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

Test is not enough complete.

  1. It should ensure that action like replace users assigned will have no impact on requesters and observers, nor on groups or suppliers.
  2. It should verify that replacement also removes actors in existing input (not only existing actors).
  3. It should verify that, when there is no existing actors, it does not fail.
  4. It should verify the ONADD case too.

You should probably have a data provider that is used to test multiple combinations.

@stonebuzz stonebuzz force-pushed the fix_rule_assign_group branch from 236c2b4 to bda3ab8 Compare November 6, 2023 10:42
@cedric-anne cedric-anne self-requested a review December 5, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants