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

[automate-1755] Reset checked projects list upon opening create… #1851

Merged
merged 3 commits into from Oct 15, 2019

Conversation

msorens
Copy link
Contributor

@msorens msorens commented Oct 11, 2019

🔩 Description: What code changed, and why?

The CreateObjectModalComponent (used for creating tokens and teams at present) appeared to retain the last set of selected projects. However, that was a phantom. They were not really selected. So upon saving the new token/team, it would show in the tokens/teams list with just (unassigned), leaving the user confused.

Two separate things to fix:
Resetting all projects on the dropdown to unchecked.
Updating the dropdown label when it is closed to reflect that no projects are checked.

⛓️ Related Resources

NA

👍 Definition of Done

Opening the "create team" (or "create token") modal for the second time in succession shows an unchecked projects list.

👟 How to Build and Test the Change

rebuild automate-ui
Create one or more projects (Settings >> Projects >> Create Project)
Create a team (Settings >> Teams >> Create Team), selecting one or more projects (1).
Save that team.
Select "Create Team" a second time (2).
Observe the projects dropdown labels should say (unassigned) and the projects dropdown list of projects should show all unchecked.

📷 Screenshots, if applicable

image
image

There were actually two pieces of state to adjust.
1. Uncheck all the projects in the dropdown upon opening the modal
for creating teams and tokens.
2. Update the label on the closed dropdown that reflects
the state of that list.

Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
@msorens msorens added bug 🐛 Something isn't working automate-auth ui automate-ui auth-team anything that needs to be on the auth team board labels Oct 11, 2019
@msorens msorens self-assigned this Oct 11, 2019
@msorens msorens requested a review from a team October 11, 2019 04:15
Copy link
Contributor Author

@msorens msorens left a comment

Choose a reason for hiding this comment

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

Some pre-review comments.

@@ -48,7 +48,8 @@ <h2 slot="title">Create {{ objectNoun | titlecase }}</h2>
<app-projects-dropdown
[projects]="projects"
(onProjectChecked)="onProjectChecked($event)"
[disabled]="dropdownDisabled()">
[disabled]="dropdownDisabled()"
[projectsUpdated]="projectsUpdatedEvent">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conduit to let the dropdown know to update the closed dropdown label.

Comment on lines -46 to +48
// if a new list of projects to populate dropdown with is passed in we update the dropdown
const checked = false;
if (hasIn('assignableProjects.currentValue', changes)) {
// update project dropdown if list changes
if (changes.assignableProjects) {
this.projects = {};
changes.assignableProjects.currentValue.forEach((proj: Project) =>
this.projects[proj.id] = { ...proj, checked });
this.projects[proj.id] = { ...proj, checked: false });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR extras: there are a variety of small cleanups such as the above in this PR unrelated to the issue being fixed.

Comment on lines +50 to +53
// clear checked projects when opening
if (changes.visible && (changes.visible.currentValue as boolean)) {
Object.values(this.projects).forEach(p => p.checked = false);
this.projectsUpdatedEvent.emit();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is half of the fix: resetting all the projects to unchecked upon opening the modal.

Comment on lines -22 to +23
(keydown.enter)="closeColumnDropdown()"
(keydown.esc)="closeColumnDropdown()"
(keydown.enter)="closeDropdown()"
(keydown.esc)="closeDropdown()"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR extra: bug fix! This function had been renamed some time ago but these were missed.

Comment on lines +44 to +49
ngOnInit(): void {
if (this.projectsUpdated) { // an optional setting
this.projectsUpdated.subscribe(() => {
this.updateLabel();
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the other half of the fix: resyncing the label to the now all-unchecked list of projects.

@@ -8,7 +8,7 @@ export const projectsFilterState = state => state.projectsFilter;
export const options = createSelector(projectsFilterState, state => state.options);

export const assignableProjects = createSelector(projectsFilterState, state => {
let projectOptions = state.options.filter((p: ProjectsFilterOption) => p.checked);
let projectOptions = (state.options as ProjectsFilterOption[]).filter(p => p.checked);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduce strong type at the start so everything else in this block is now typed, too.

@msorens msorens marked this pull request as ready for review October 11, 2019 04:21
Copy link
Contributor

@susanev susanev left a comment

Choose a reason for hiding this comment

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

works well, thanks for fixing this!!

Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@msorens msorens changed the title [automate-1755] Reset checked projects list upon opening create modal [automate-1755] Reset checked projects list upon opening create… Oct 15, 2019
@msorens msorens merged commit 4506f24 into master Oct 15, 2019
@msorens msorens deleted the automate-1755 branch October 15, 2019 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth-team anything that needs to be on the auth team board automate-auth automate-ui bug 🐛 Something isn't working ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants