Skip to content

Conversation

@mwtrew
Copy link
Contributor

@mwtrew mwtrew commented Oct 20, 2025

Re-enables required account info modal on Ada CS and centralises logic for which fields are required in validateFields, meaning the same logic is used for deciding which fields to show and checking the validity of the user object on submission. This creates a single source of truth for which fields are required across both sites, which will hopefully prevent validation and presentation logic from drifting apart in the future.

validateFields could also be used to simplify validation in signup and My Account, but I haven't included that in this PR. I will create a card for that.

Also replaces the old "big red text"-style error in the RAI modal with an ExigentAlert on both sites, which looks nicer.

@mwtrew mwtrew changed the title Ada enable required account info modal Re-enable required account info modal on Ada CS Oct 20, 2025
@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

❌ Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.62%. Comparing base (423edfa) to head (daa8ed6).
⚠️ Report is 113 commits behind head on main.

Files with missing lines Patch % Lines
...lements/modals/RequiredAccountInformationModal.tsx 27.27% 8 Missing ⚠️
src/app/services/validation.ts 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1789      +/-   ##
==========================================
+ Coverage   41.58%   41.62%   +0.03%     
==========================================
  Files         535      534       -1     
  Lines       23509    23522      +13     
  Branches     6933     7786     +853     
==========================================
+ Hits         9776     9790      +14     
+ Misses      13692    13691       -1     
  Partials       41       41              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mwtrew mwtrew marked this pull request as ready for review October 21, 2025 08:53
Copy link
Contributor

@jacbn jacbn left a comment

Choose a reason for hiding this comment

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

This looks great! I like the validity object. Couple of thoughts (not to action for this):

  • When signing in with SSO currently we don't guide the user towards making a teacher account if that is what they want; they need to manually go to the teacher upgrade page. Given that this modal appears right after they first return to the site after completing SSO, the flow is now SSO => fill in RAI modal => upgrade to teacher => fill in RAI modal again for the teacher props (provided it shows up? the 50 mins before it appears probably won't have passed). It seems to me that after SSO but before the first appearance of the RAI modal we should have a student / teacher account type selection screen, but we can discuss this separately.
  • When the form is invalid (particularly if the invalid field is one of the top ones), the error at the top is nice but can be hidden due to scrolling as the modal is quite tall. I would like to move to the other invalid model to fix this (this auto-scrolls to erroring fields), but again we should also be doing this on My Account so this can be separate.

@jacbn jacbn merged commit 5ff7bd0 into main Oct 21, 2025
10 checks passed
@jacbn jacbn deleted the ada-enable-required-account-info-modal branch October 21, 2025 10:58
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