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

Make Save button respond properly on changes in User's Available Groups #6369

Conversation

hstastna
Copy link

@hstastna hstastna commented Nov 1, 2019

Issue: #6364

In User's editing screen, Save button wasn't responding properly on changes in Available Groups section, especially if there were more groups selected in the drop down.

Another issue was improper order of groups in Selected Groups section while adding/editing User. It was inconsistent with the User's details screen where the Groups are properly ordered (by description).

This PR fixes both issues, together with fixing two improper comments in the code.

Details:
The improper response of Save butto on changes in User's Available Groups was caused by sorting the Groups' ids represented by array of strings. We need to put the ids from strings to integers AND THEN to sort the array of ids. We need to have ids in both @edit[:new][:group] and @edit[:current][:group] sorted to be able to compare them and then to set changed variable properly, for proper response of Save button.

The second issue is fixed simply by adding .sort to the array of descriptions of selected groups. I've also removed unnecessary code in the same haml, as @edit[:new][:group] is already edited here before it is used in the haml, so no need to do the same stuff again in the haml, I mean splitting the string (@edit[:new][:group] is already an array of integers in the haml, not string!), and checking for 'null'.


User's details screen, check the Groups and their order - groups are alphabetically orderd:
users_detail

Before: bad response of Save button and bad order of Selected Groups
user_before

After:
user_after

@hstastna hstastna force-pushed the Save_button_respond_UsersAvailableGroups branch 4 times, most recently from 01fdd94 to f78f323 Compare November 4, 2019 10:13
@ZitaNemeckova
Copy link
Contributor

ZitaNemeckova commented Nov 18, 2019

@hstastna The issue with Save button is still present in this PR. PR is fixing Ordering.

@hstastna hstastna force-pushed the Save_button_respond_UsersAvailableGroups branch from f78f323 to 827a121 Compare November 18, 2019 15:09
@hstastna
Copy link
Author

Moving this PR to WIP as it fixes the issue only for some situations, not always.

@hstastna hstastna changed the title Make Save button respond properly on changes in User's Available Groups [WIP] Make Save button respond properly on changes in User's Available Groups Nov 18, 2019
@miq-bot miq-bot added the wip label Nov 18, 2019
@hstastna hstastna force-pushed the Save_button_respond_UsersAvailableGroups branch 3 times, most recently from d0d4801 to f0cdb80 Compare November 20, 2019 12:54
@hstastna hstastna changed the title [WIP] Make Save button respond properly on changes in User's Available Groups Make Save button respond properly on changes in User's Available Groups Nov 20, 2019
@hstastna
Copy link
Author

@miq-bot add_reviewer @ZitaNemeckova
Zita, I've added one more little change, and now the PR should work perfectly.

@miq-bot miq-bot removed the wip label Nov 20, 2019
@ZitaNemeckova
Copy link
Contributor

Tested in UI. LGTM 👍

@hstastna hstastna force-pushed the Save_button_respond_UsersAvailableGroups branch from f0cdb80 to 4ada34d Compare November 27, 2019 10:34
@miq-bot
Copy link
Member

miq-bot commented Nov 27, 2019

Checked commits hstastna/manageiq-ui-classic@2003cd8~...4ada34d with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

@martinpovolny martinpovolny self-assigned this Dec 4, 2019
@martinpovolny martinpovolny added this to the Sprint 126 Ending Dec 9, 2019 milestone Dec 4, 2019
@ZitaNemeckova
Copy link
Contributor

Re-tested. LGTM 👍

@martinpovolny
Copy link
Member

Thank you both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants