Skip to content

Conversation

stonebuzz
Copy link
Contributor

Checklist before requesting a review

Please delete options that are not relevant.

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

Since the UI refactor of rules:

#17203

The form for authorization rules contains two additional fields that do not work and do not appear to be used. Their presence in this location seems rather odd.

image

An initial inconsistency had been detected and fixed here:

https://github.com/glpi-project/glpi/pull/19656/files#diff-186234e2e73b65f55a4e8779745ad83d0863119d64b860b3f80260509049f3d0

However, these two fields are now displayed systematically.

Screenshots (if appropriate):

@stonebuzz stonebuzz requested a review from cconard96 September 29, 2025 09:09
@stonebuzz stonebuzz self-assigned this Sep 29, 2025
@stonebuzz stonebuzz added the bug label Sep 29, 2025
@trasher
Copy link
Contributor

trasher commented Sep 29, 2025

I do not know if those fields are used or not, but they were present before Twig migration

@cedric-anne
Copy link
Member

I do not know if those fields are used or not, but they were present before Twig migration

It seems that they were used only in a specific case:
image

The condition on short removed in #19656 must probably be restored and its lack of definition handled properly.

@cedric-anne cedric-anne added this to the 11.0.0 milestone Sep 29, 2025
'match_operators' => $this->getRulesMatch(),
'conditions' => static::getConditionsArray(),
'rand' => mt_rand(),
'short' => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should only be the "short" form in the context of the Rules tab in the Entity form I think.

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.

See @cconard96 comment.

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.

As far as I understand, to restore the behaviour of GLPI 10.0, you have to invert conditions. Also, the variable name is ambiguous, so it should be renamed.

@trasher
Copy link
Contributor

trasher commented Sep 29, 2025

@cedric-anne variable name is still short, did you miss a pull before you force push?

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.

In fact, the required changes could be more complex.

Before #17203, the rules forms displayed in the entity tab were processed by the Entity::executeAddRule() method. This is no longer the case. Maybe it should be restored, but I do not really know how it is supposed to work.

@cedric-anne
Copy link
Member

@cedric-anne variable name is still short, did you miss a pull before you force push?

I reverted, it is used elsewhere.

Copy link
Contributor

@cconard96 cconard96 left a comment

Choose a reason for hiding this comment

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

The form in the context of Entities is still broken functionally. I don't really understand the usability of the extra fields, but any RuleRight created from here is done with no criteria or actions.

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.

4 participants