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

Fix scope - only tickets are affected #267

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

Conversation

keguira
Copy link

@keguira keguira commented Dec 9, 2024

"Display delete button" affect only ticket form not the change and problem.

fixes #266

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

Screenshots (if appropriate):

"Display delete button" affect only ticket form not the change and problem.

fixes pluginsGLPI#266
@stonebuzz
Copy link
Contributor

@keguira thanks for contribution

I'm not sure what to think, it seems like it's intentional to have the behavior on the change and problem since 2.2.2

https://github.com/pluginsGLPI/escalade/releases/tag/2.2.2

@Rom1-B what do you think?

for me this PR is legitimate

It would be necessary (not necessarily in this PR) to have a dedicated option for change and problem)

@keguira
Copy link
Author

keguira commented Dec 10, 2024

@keguira thanks for contribution

I'm not sure what to think, it seems like it's intentional to have the behavior on the change and problem since 2.2.2

https://github.com/pluginsGLPI/escalade/releases/tag/2.2.2

@Rom1-B what do you think?

for me this PR is legitimate

It would be necessary (not necessarily in this PR) to have a dedicated option for change and problem)

Hello, I though too at first but as escalate does not handle changes and problems, this sugar-feature should not be added on these form too. Also, all texts and the vocabulary states it's for tickets.

I think the first intention was to also allow the escalation of these elements but it was not implemented (or lost with time). Maybe we can add the escalation feature to changes and problems and add dedicated switches for them ? Whatever we decide, the current one should only affect tickets

Edit: also, i forgot to do it, i updated the changelog in a second commit. I can rebase if you want

added changelog information for the fix
Copy link
Contributor

@Rom1-B Rom1-B left a comment

Choose a reason for hiding this comment

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

It seems that the documentation was not updated, as this functionality is also supposed to work for changes and problems. Therefore, it would be better to fix this part rather than removing it only to reintroduce it later (which would likely involve undoing your changes). I believe this is the most efficient approach.

fd286fa?w=1#diff-497804e83ceb478d4b3ec788864fa8cf4268fec92f908da67f7c8918bb35ef0dR68

@keguira
Copy link
Author

keguira commented Dec 11, 2024

It seems that the documentation was not updated, as this functionality is also supposed to work for changes and problems. Therefore, it would be better to fix this part rather than removing it only to reintroduce it later (which would likely involve undoing your changes). I believe this is the most efficient approach.

fd286fa?w=1#diff-497804e83ceb478d4b3ec788864fa8cf4268fec92f908da67f7c8918bb35ef0dR68

I could not agree more with you.

From what I see, my changes are the only two places where we have a reference to problem and changes.

Other places in setup and even configurations are ticket only even from a security point of view.

For example :
setup.php

if (
    $_SESSION['glpiactiveprofile']['interface'] == "central"
    && (Session::haveRight("ticket", CREATE)
        || Session::haveRight("ticket", UPDATE))
) {

assign_me.php

if (! isset($_REQUEST['tickets_id'])) {
    Html::displayErrorAndDie(__("missing parameters", "escalade"));
}

PluginEscaladeTicket::assign_me((int) $_REQUEST['tickets_id']);

I think it may have been intended at one point but the functionality was lost since the commit in 2017 and GLPI 10.

Fixing it for changes and problems is not a bugfix but a full implementation that has to be done. In this case, my first guess would be to fix the current issue first and rework later (as we will also have to work on GLPI 11 compatibilities issues).

The escalade function may have to be implemented for both of these concepts and by adding them, I guess it will have an impact on every existing things during refactor and simplification.

I can do the implementation but I will need the community help and it will takes me/us time and the bug will still be there until release.

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.

Display delete button is also applied on changes and problems
3 participants