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

WebUI: fix showing required asterisk '*' #713

Closed
wants to merge 1 commit into from

Conversation

pvomacka
Copy link

@pvomacka pvomacka commented Apr 13, 2017

There was a bug that when user switch between two facets where is
required field and in one of them is writable and in second one
is not writable, then the asterisk which marks required field is
not shown. i.e. admin vs. user details page or global_passwd_policy
vs. other_passwd_policy details page.

That was caused by incorrect evaluation of required state of field.
Evaluation works that way: evaluate old required state, then evaluate
current required state and if states has changed then emit change event.
The evaluation depends on writable and read_only state of field.
Those two states are set before evaluation of required state, but
their old values (for evaluating previous required stated) were
not stored anywhere.

This commit adds two attributes which stores old writable
and read_only states. The required asterisk is then shown correctly.

https://pagure.io/freeipa/issue/6849

@pvomacka pvomacka requested a review from pvoborni April 13, 2017 15:27
@pvoborni
Copy link
Member

I was thinking about signature of is_required(old) method. It is not very clear what it does from reading the code if we forget context of this patch, especially if we see: var old = that.is_required(true). There is probably similar thing in the code already implemented by me. But I was thinking if it should be rather changed into two methods: is_required() and was_required. Then the readonly or writable will use calls like var old = that.was_required(); and var required = that.is_required();.

@pvomacka pvomacka added the re-run Trigger a new run of PR-CI label Aug 24, 2017
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Aug 24, 2017
@pvoborni pvoborni added the re-run Trigger a new run of PR-CI label Aug 30, 2017
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Aug 30, 2017
@pvoborni pvoborni self-assigned this Aug 31, 2017
There was a bug that when user switch between two facets where is
required field and in one of them is writable and in second one
is not writable, then the asterisk which marks required field is
not shown. i.e. admin vs. user details page or global_passwd_policy
vs. other_passwd_policy details page.

That was caused by incorrect evaluation of required state of field.
Evaluation works that way: evaluate old required state, then evaluate
current required state and if states has changed then emit change event.
The evaluation depends on writable and read_only state of field.
Those two states are set before evaluation of required state, but
their old values (for evaluating previous required stated) were
not stored anywhere.

This commit adds two attributes which stores old writable
and read_only states. The required asterisk is then shown correctly.

https://pagure.io/freeipa/issue/6849
@pvomacka
Copy link
Author

@pvoborni the change you are proposing makes sense, I changed the code that way. Thank you

@pvoborni pvoborni added the ack Pull Request approved, can be merged label Sep 1, 2017
@tkrizek
Copy link
Contributor

tkrizek commented Sep 1, 2017

master:

  • d5ef1a7 WebUI: fix showing required asterisk '*'

@tkrizek tkrizek added the pushed Pull Request has already been pushed label Sep 1, 2017
@tkrizek tkrizek closed this Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants