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

No-Jira Fix Campus #847

Merged
merged 9 commits into from
Nov 9, 2023
Merged

No-Jira Fix Campus #847

merged 9 commits into from
Nov 9, 2023

Conversation

caleballdrin
Copy link
Contributor

Description

Students are submitting ERT forms with campuses that do not match the campus names in Infobase. This is because some browsers are auto-filling the campus data. The UI only validates when the user changes the field. So this bad data gets submitted.

Changes I made

  • On page load, check if there is an answer already in the field. If there is, check to make sure that exists in the database with the searchCampuses() function. If it doesn't, set the answer to ''.
  • Every time the UI validates the fields, include the campus question in that validation to make sure it is not empty.

@caleballdrin caleballdrin added the On Staging Will be merged to the staging branch by Github Actions label Nov 6, 2023
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

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

One stray comment, but it looks great! Good work with the tests too.

@canac
Copy link
Contributor

canac commented Nov 7, 2023

Unless it's on purpose, you probably want to revert all your changes to package.json and yarn.lock. I believe that git checkout master -- package.json yarn.lock should do it.

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

When testing on staging, my autofill still works. I was able to get to the final stage of the register form.

Screenshot 2023-11-08 at 8 31 21 AM

app/scripts/directives/blocks.js Outdated Show resolved Hide resolved
@caleballdrin
Copy link
Contributor Author

@dr-bizz I haven't pushed it to staging yet. The On Staging label doesn't work for the ERT. I'm not sure why. It would be nice if it did.

@caleballdrin
Copy link
Contributor Author

@dr-bizz It is on staging now.

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Just tested it again and this is still happening

Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

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

There's one more tweak we should make to the test. I think that if you use my suggestion, lines 257-258 will be hit by code coverage. We also might need a test with a non-empty campus answer in validateRegistrant.spec.js to cover line 343.

app/scripts/directives/blocks.js Outdated Show resolved Hide resolved
test/spec/directives/block.spec.js Show resolved Hide resolved
Copy link
Contributor

@canac canac 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 now! 💯

@caleballdrin caleballdrin merged commit 98a8b38 into master Nov 9, 2023
14 checks passed
@caleballdrin caleballdrin deleted the fix-campus branch November 9, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants