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

Fix data leak when selecting anonymous address type #108

Merged
merged 3 commits into from Jun 28, 2023
Merged

Conversation

Sperling-0
Copy link
Contributor

Copy link
Member

@Abban Abban 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, and it's the right way to do it. I just have a couple of things you should do.

  • Email only address types send only the email/salutation/title/firstName/lastName so you should split the if statement into 2:
    1. One that checks the address type is email or full and includes the email/salutation/title/firstName/lastName fields.
    2. One that checks the address type is full and includes the other address fields.
  • You should add tests to SubmitValues.spec.ts that make sure the correct fields are present for the each address type.

@Abban Abban self-requested a review June 7, 2023 13:11
Copy link
Member

@Abban Abban left a comment

Choose a reason for hiding this comment

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

Normally I try to not nest if statements, but this looks okay to me.

You can fix the CI by running npm run test:unit -- -u to clean up the snapshots.

Copy link
Member

@gbirke gbirke left a comment

Choose a reason for hiding this comment

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

Thank you for picking this up! While reviewing, I was a bit confused about the nested <template> tags. Maybe you can rework this, by creating two Boolean computed properties, sendPostalAddress and sendNameAndEmail, wrapping the input fields in 2 non-nested template tags that check for these variables. It would also move the comparison logic out of the template section and into the script section, which I like better

@gbirke
Copy link
Member

gbirke commented Jun 12, 2023

Thank you for incorporating my suggestions!

@moiikana moiikana merged commit d46402c into main Jun 28, 2023
3 checks passed
@moiikana moiikana deleted the fix-anonymous branch June 28, 2023 13:44
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.

None yet

4 participants