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-3030] Enhance edits pending bar to show failure #3091

Merged
merged 8 commits into from
Mar 13, 2020

Conversation

msorens
Copy link
Contributor

@msorens msorens commented Mar 13, 2020

🔩 Description: What code changed, and why?

When a project update fails, we want to make that much more apparent to the user.
This replaces the standard "edits pending" message with an error message (and coloring) that makes it evident to the user.

The illustration shows the two possible states of the pending edits bar, showing both wording and color change, along with the subtle color change of the button itself when hovering or focusing it.

image

⛓️ Related Resources

NA

👍 Definition of Done

Here is the contract as defined by unit tests. Most of these were already in place. The new tests are after the ----- divider.

 update-start-confirmation modal...
   when it emits a cancellation, hides the modal
   when it emits a confirmation, hides the modal
   when it emits a confirmation, the projectService starts the rule updates
 pending edits bar...
   is hidden if no project has changes
   is visible if some project has changes
   is visible if some project has changes... UNLESS progress bar is active
   is visible if rules are not being applied
   is visible if update fails
   is visible if update is cancelled
   is visible if update is cancelled but update is still running
----
   displays edits pending message before update is applied
   displays edits pending message if an update is cancelled
   displays failed message if an update fails
   displays failed message if both an update fails and was cancelled

👟 How to Build and Test the Change

In hab studio: rebuild components/authz-service

Remove the last commit to expose a testing harness:

git revert -n 3df50d5

Then rebuild automate-ui per your preferred method.
Create a project and a rule in the browser.
Navigate to the projects page to expose the edits pending bar in its nominal condition (due to another issue, the edits pending bar will not start appearing until you visit that particular page).
Now just watch: every 5 seconds a random failure--or not--will occur. You should see the blue (normal) or pink (failure) bars both appear over time.

That shows the "normal" edits pending state--before an update is applied,
and the result of a failed update. To see the result of a cancelled update, first you have to reset the code--pull down a fresh copy of the branch so it no longer has that revert mentioned above. Then just proceed normally: start an update and cancel it. You will see the normal edits pending bar.

✅ Checklist

@msorens msorens changed the title Enhance edits pending bar to show failure [automate-3030] Enhance edits pending bar to show failure Mar 13, 2020
@msorens msorens self-assigned this Mar 13, 2020
@msorens msorens added auth-team anything that needs to be on the auth team board automate-auth automate-ui ui labels Mar 13, 2020
@msorens msorens added this to the Auth: Sprint 10 milestone Mar 13, 2020
@msorens msorens marked this pull request as ready for review March 13, 2020 14:30
@msorens msorens requested review from a team March 13, 2020 14:30
Comment on lines +2 to +8
<div [className]="updateProjectsFailed ? 'failure-container' : 'normal-container'">
<span *ngIf="!updateProjectsFailed" class="info">
Project edits pending, update <a [routerLink]="['/settings/projects']">projects</a> to apply changes.
</span>
<span *ngIf="updateProjectsFailed" class="info">
Project update failed. Review system status from the command line with <pre>chef-automate status</pre> and try again.
</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike the separation of pending edits vs. update in progress, that really have different functionality, the distinction between pending edits and pending-edits-due-to-failed-update is really the same functionality. That is why I just made this component handle both variations rather than creating a new component (which would have addded significant overhead for its infrastructure in layout facade).
In fact, here we already have the variable we need to distinguish--this.updateProjectsFailed.

Comment on lines +8 to +14
.normal-container {
padding: 16px 10px;
}

.failure-container {
padding: 16px 10px;
background-color: $chef-critical;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two are in a new child div of pending-edits-bar, which starts with the default blue background. If it is a normal-container, that does not change. But if it is a failure-container, that completely overlays the blue background with the pink background.

Comment on lines +39 to 46
.failure-container button:hover,
.failure-container button:focus {
background-color: $chef-critical-hover;
}

.normal-container button:hover,
.normal-container button:focus {
background-color: $chef-primary-hover;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides the background color, we also want the hover and focus colors to adjust.

Comment on lines +129 to +132
const payload: GetApplyRulesStatusSuccessPayload = {
...action.payload,
percentage_complete: action.payload.percentage_complete * 100
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting this in a variable provides strong typing.

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.

if a user takes the stop action they should not see the failed banner, as discussed over slack.
otherwise, this looks good.

Copy link
Contributor

@tylercloke tylercloke left a comment

Choose a reason for hiding this comment

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

This is really coming along! Nice improvement.

Copy link
Contributor

@SEAjamieD SEAjamieD left a comment

Choose a reason for hiding this comment

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

Working great 🎉

msorens added 5 commits March 13, 2020 14:08
Applies different text and colors triggered
 by a failure returned upon checking the update status.

Signed-off-by: michael sorens <[email protected]>
This commit, when exposed, will update the UI
every 5 seconds with a random selection of
failed or not failed project rule status.
Thus, the pending edit bar will change from one form
to the other every 5 seconds on average.

Signed-off-by: michael sorens <[email protected]>
This reverts commit 716baf8,
a commit that was present for testing only.
With this reversion in place, it is safe to merge this pull request.
Turns out the new behavior revealed a sub-optimal condition:
a cancel operation also results in a "failure" condition with message "Canceled".
This disambiguates a cancel by suppressing the failed status,
so the UI responds more clearly.

Signed-off-by: michael sorens <[email protected]>
@msorens msorens force-pushed the automate-3030/project-update-fail-msg branch from 3df50d5 to e7fd65e Compare March 13, 2020 21:08
Signed-off-by: michael sorens <[email protected]>
@msorens
Copy link
Contributor Author

msorens commented Mar 13, 2020

@susanev Displaying the failure banner on cancel-project-update was not a bug in this PR; it just exposed that bug that we had not known about in the service. I adjusted the authz-service to correct that.

Brenna Hewer-Darroch added 2 commits March 13, 2020 15:23
the start and updated times were some dates grabbed from the perf env,
so once those dates passed, FindSlowestJobStatus
started returning null time and failing the test

Signed-off-by: Brenna Hewer-Darroch <[email protected]>
Signed-off-by: Brenna Hewer-Darroch <[email protected]>
@msorens msorens merged commit 741bd01 into master Mar 13, 2020
@msorens msorens deleted the automate-3030/project-update-fail-msg branch March 13, 2020 23:33
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 ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants