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

PP-7843 Stop selecting default option on permission level radios #4148

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

hjvoid
Copy link
Contributor

@hjvoid hjvoid commented Oct 19, 2023

WHAT

  • Removed the checked: true property from the role-input checkboxes.
  • Stopped the error being passed to the try/catch block and implemented a more explicit flash error message for the user.
  • Added a test to check that the correct error message is displayed if the role-id is not recognised.
  • Changed the property on the invite-user ui test to check that none of the role inputs should be checked when the page is loaded.

HOW

  • When a user invites a new team member, no user role should be selected.
  • If the user does not check on of the checkboxes before they submit the form they are presented with an error which reads 'Select the team member's permission level'.

Copy link
Contributor

@alexbishop1 alexbishop1 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. Just one thing about an apostrophe.

@@ -60,7 +60,9 @@ async function invite (req, res, next) {
lodash.set(req, 'session.pageData', { invitee })
res.redirect(303, formatServicePathsFor(paths.service.teamMembers.invite, externalServiceId))
} else if (!role) {
next(new Error(`Cannot identify role from user input ${roleId}`))
req.flash('genericError', 'Select the team member\'s permission level')
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use a right single quotation mark (that is, instead of ') then it looks nicer and does not need to be escaped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! Have updated this as above.

@@ -64,7 +64,7 @@
"chai-arrays": "2.2.0",
"chai-as-promised": "7.1.1",
"cheerio": "1.0.0-rc.12",
"chokidar-cli": "*",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be excluded unless we’re actually trying to update dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason why we can't update chokidar? I see this change every time I npm i so it'd be nice to merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted this back to latest for the time being as I guess it's not part of the story?

Copy link
Contributor

@alexbishop1 alexbishop1 left a comment

Choose a reason for hiding this comment

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

One final thing with the package-lock.json file and this is good to go.

@@ -128,7 +128,7 @@
"chai-arrays": "2.2.0",
"chai-as-promised": "7.1.1",
"cheerio": "1.0.0-rc.12",
"chokidar-cli": "latest",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be * (what it is on master), not latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh...Missed that!

@hjvoid hjvoid merged commit 7f524f8 into master Oct 23, 2023
5 checks passed
@hjvoid hjvoid deleted the PP-7843_Remove_Default_Checked_Radio_Buttons branch October 23, 2023 11:46
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