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

Remove user deletion when we add more than one user #269

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

Conversation

Lainow
Copy link
Contributor

@Lainow Lainow commented Dec 18, 2024

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

  • It fixes !35657
  • Here is a brief description of what this PR does

When we try to add several technicians to a ticket with the escalation plugin enabled and the Remove technician(s) on escalation option active, only 1 is assigned.

Screenshots (if appropriate):

Screencast-from-18-12-2024-15_03_20

@Lainow Lainow requested review from stonebuzz and Rom1-B December 18, 2024 14:05
@@ -622,7 +622,6 @@ public static function item_add_user(Ticket_User $item, $type = CommonITILActor:
$ticket->getFromDB($tickets_id);
$groups_id = [];

self::removeAssignUsers($ticket, $users_id, $type);
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 the value of $users_id? Does it properly include the two actors in your example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it takes care of users 1 by 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Simply removing the call to removeAssignUsers() might cause regressions elsewhere.
As suggested by @stonebuzz, please perform a git bisect, and tests would be welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

apparently broken behaviour since version 2.9.8

@stonebuzz
Copy link
Contributor

Can you do a git-bisect(between 2.9.8 and 2.9.10 to know which commit would have broken the option

(from memory that part had been reworked #219)

Mayve the related commit is involved

@Lainow Lainow force-pushed the fix-user-deletion branch 7 times, most recently from e4c9744 to 08e2fe7 Compare December 19, 2024 11:10
Copy link
Contributor

@stonebuzz stonebuzz left a comment

Choose a reason for hiding this comment

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

Can you adapt CHANGELOG.md

@Lainow Lainow force-pushed the fix-user-deletion branch 6 times, most recently from bd4c694 to cf55101 Compare December 19, 2024 14:26
@Lainow Lainow requested review from stonebuzz and Rom1-B December 19, 2024 14: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