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

[EASI-4608] Start GRB review #2821

Open
wants to merge 24 commits into
base: feature/EASI-4325
Choose a base branch
from

Conversation

aterstriep
Copy link
Contributor

@aterstriep aterstriep commented Sep 19, 2024

EASI-4608

Figma

Description

  • Added Start GRB Review button to GRB Review tab
    • Button shows for IT Gov admins if review has NOT been started
    • If review has been started, start date shows for all GRB reviewers
  • Added alert above participants table if review has not been started
  • Updated alert text on Add GRB Reviewer form
  • Unit tests

Note

Seeded data is broken and CI tests will fail until #2814 is merged into the feature branch.

How to test this change

  • Log in as IT Gov admin
  • Complete new intake request
  • Go to GRB review tab (/it-governance/:systemIntakeId/grb-review)
  • Check that alert text matches Figma
  • Click "Start GRB Review"
  • Page should update to hide button and show review start date

PR Author Checklist

  • I have provided a detailed description of the changes in this PR.
  • I have provided clear instructions on how to test the changes in this PR.
  • I have updated tests or written new tests as appropriate in this PR.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • When approving a PR, provide a reason why you're approving it
    • e.g. "Approving because I tested it locally and all functionality works as expected"
    • e.g. "Approving because the change is simple and matches the Figma design"
  • Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

@aterstriep aterstriep marked this pull request as ready for review September 19, 2024 18:52
@aterstriep aterstriep requested a review from a team as a code owner September 19, 2024 18:52
@aterstriep aterstriep requested review from adamodd and removed request for a team September 19, 2024 18:52
@adamodd
Copy link
Contributor

adamodd commented Sep 19, 2024

Not sure if lint is failing here for something similar I had, but I manually ran something like:
pre-commit run --all-files
to fix a similar error, even though I had merged in and npm reinstalled packages after the linter/prettifier upgrade.

Edit: Just noticed this was a feature merge, not main. So maybe this does just need an update from main.

@aterstriep aterstriep requested a review from a team as a code owner September 20, 2024 16:18
@aterstriep aterstriep requested review from downeyn-cms and removed request for a team September 20, 2024 16:18
Copy link
Contributor

@adamodd adamodd left a comment

Choose a reason for hiding this comment

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

Changes look good.

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

Successfully merging this pull request may close these issues.

3 participants