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

feat(admin-form): support nric masking #7388

Merged
merged 37 commits into from
Jun 27, 2024
Merged

feat(admin-form): support nric masking #7388

merged 37 commits into from
Jun 27, 2024

Conversation

kevin9foong
Copy link
Contributor

@kevin9foong kevin9foong commented Jun 12, 2024

Problem

Support Nric masking for e-voting.
Closes FRM-1736

Solution

Implement functionality and components as required.

Features:

Unit testing

  • backend unit tests in src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.controller.spec.ts & src/app/modules/submission/email-submission/__tests__/email-submission.controller.spec.ts are written. these TC cover the nric mask enabled case and nric mask disabled case.

Breaking Changes

  • No - this PR is backwards compatible

Features:

  • mask nric
  • frontend changes based on design master figma

Improvements:

  • refactoring of code (removing usememo, making code terse)

Before & After Screenshots

Admin POV

Component Before After
Singpass Auth Settings when toggle off image
radio options directly in settings auth page, old toast message
image singpass auth turned off, further settings hidden.
Singpass Auth Settings when toggle on See above image image turn on and select singpass auth method + nric masking enabled
Disabled toggles image when form is open, admin cannot edit any singpass settings image when myinfo fields are added, nric masking is enabled but singpass auth settings are disabled.
Form builder wizard image admin sees that sample NRIC is masked when nric toggle is on
NRIC masking screens
Storage mode masking image
email mode masking image
webhook masking image Does not mask the myinfo data, nric data is masked

Respondent POV

Component Before After
PublicForm imagerespondent sees masked nric is applied

Tests

Test email mode nric should be masked when nric mask toggle is on:

  • create an email mode form
  • ensure the form builder initially shows unmasked sample nric (S1234567A)
  • go to the singpass tab, turn on the enable singpass auth toggle, select any one of the singpass auth methods
  • toggle on nric masking
  • ensure the form builder shows sample masked nric (****567A)
  • open the form to public submission (in general settings tab)
  • respond to the form
  • check form response in email mailbox, verify the nric is masked
  • turn off the nric masking
  • make another form response
  • check form response in email mailbox, verify nric is not masked

Test storage mode nric should be masked when nric mask toggle is on + myinfo fields are not masked despite nric masking enabled + settings auth options are correctly disabled/enabled based on form state:

  • create a storage mode form
  • ensure the form builder initially shows unmasked sample nric (S1234567A)
  • go to the singpass tab, turn on the enable singpass auth toggle, select sgid-myinfo
  • toggle on nric masking
  • ensure the form builder shows sample masked nric (****567A)
  • add any 2 myinfo field to form (eg, name and country)
  • open the form to public submission (in general settings tab)
  • respond to the form
  • check form responses in admin panel, verify the nric is masked and the myinfo fields are not masked
  • make form private
  • ensure that the nric masking toggle can be turned off but radio button is disabled (due to presence of myinfo fields)
  • turn off the nric masking
  • make another form response
  • check form response in admin panel, verify nric is not masked

@kevin9foong kevin9foong requested a review from KenLSM June 12, 2024 03:36
@kevin9foong kevin9foong self-assigned this Jun 12, 2024
Copy link

linear bot commented Jun 12, 2024

@kevin9foong kevin9foong force-pushed the feat/nric-masking branch 5 times, most recently from 9396389 to c87e54a Compare June 16, 2024 13:30
@kevin9foong
Copy link
Contributor Author

kevin9foong commented Jun 16, 2024

The core functionality is complete

  • masking (done at the backend at forms GET and submissions POST), toggle button, duplication

Remaining works are:

  • code quality improvement/refactoring of the code
  • discussion on whether frontend masking at the PublicFormProvider is needed
  • making the setting UI page reflect the design docs (& implementing the new toggle for the singpass auth)
  • writing tests

@kevin9foong kevin9foong force-pushed the feat/nric-masking branch 3 times, most recently from bffe92b to aec8fcd Compare June 21, 2024 08:42
@kevin9foong kevin9foong marked this pull request as ready for review June 24, 2024 14:26
@kevin9foong
Copy link
Contributor Author

Ready for final review. pending design approval on chromatic

frontend/public/index.html Outdated Show resolved Hide resolved
@kevin9foong kevin9foong requested a review from KenLSM June 27, 2024 02:43
Copy link
Contributor

@KenLSM KenLSM left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @kevin9foong !

@kevin9foong kevin9foong merged commit 79106eb into develop Jun 27, 2024
21 of 22 checks passed
@kevin9foong kevin9foong deleted the feat/nric-masking branch June 27, 2024 06:45
@KenLSM KenLSM mentioned this pull request Jun 30, 2024
30 tasks
g-tejas pushed a commit that referenced this pull request Jul 18, 2024
* feat(admin-form): add frontend nric mask toggle to singpass auth settings

* fix(admin-form): fix spacing and change units from px to rem

* feat(admin-form): create isNricMaskingEnabled field in database when form is created

* feat(admin-form): enable fetching and updating isNricMaskingEnabled field in backend routes

* feat(admin-form): connect frontend component with server api routes

* fix(admin-form): make code terser, remove unused params, fix nullish-coalesce operator usage

* fix(admin-page): fix flickering bug

* feat(duplicate-api): implement duplication of isNricMaskEnabled field

* feat(public-form): prevent nric from reaching frontend if masked

* feat(nric-mask): apply masking when submitting forms

* feat(nric-mask): update frontend template to reflect masking toggle changes

* fix(index.html): checkout to develop branch

* fix: remove console.log

* fix(toggle-component): replace nullish coalesce with ternary operator

* chore(public-form-controller): remove masking when fetching public form at backend

* fix: refactor email-submission-controller to mask at single location

* fix: clean up nric-mask util function and unused imports

* fix: update field with evaluated map value

* feat: add nric masking to the frontend public form provider

* feat: remove tooltip and labelComponentRight from Toggle to match design

* feat: update settings auth page to reflect design master

* feat: add err message

* feat: change px to rem

* feat: disable independently for nric masking and auth toggle

* fix: change MyInfo to Myinfo in description message

* fix: update form model test suite to expect new isNricMaskEnabled field

* feat: add tests for nric mask util and masking for email submission

* feat: add test for isNricMaskDisabled for email submission

* feat: add tests for storage mode submission masking

* fix: fix bug with submission not being able to save authType and defaulting to nil

* feat: add stories for updated settings auth page

* feat: update playwright test helper to support new auth setting flow

* fix: fix review comments and run frontend lint fix

* fix: fix based on chromatic review

* fix: fix review comments

* feat: add testing for getDuplicateParams to ensure that nricmask is correctly duplicated

* refactor: extract skeleton component out to own file

---------

Co-authored-by: Ken <[email protected]>
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.

2 participants