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

[chore] Simplify email types #4206

Merged
merged 17 commits into from
Aug 22, 2024
Merged

[chore] Simplify email types #4206

merged 17 commits into from
Aug 22, 2024

Conversation

elland
Copy link
Contributor

@elland elland commented Aug 12, 2024

This factors out one of our email types in favour of a validated email, this means a lot of tests that created arbitrary bytestrings became invalid and were replaced with simple valid emails, this might have caused redundancy in some of the golden tests.

https://wearezeta.atlassian.net/browse/WPB-665

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 12, 2024
@elland elland force-pushed the elland/saml-email branch from 79f152a to 1a6ca21 Compare August 13, 2024 09:21
@echoes-hq echoes-hq bot added the echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. label Aug 13, 2024
@elland elland force-pushed the elland/saml-email branch from 5048386 to cd83f4d Compare August 14, 2024 15:30
@elland elland changed the title Handle emails field in  SCIM user record [chore] Refactor email types to remove intermediate representation. Aug 19, 2024
@elland elland changed the title [chore] Refactor email types to remove intermediate representation. [chore] Simplify email types Aug 19, 2024
@mdimjasevic mdimjasevic self-requested a review August 19, 2024 08:59
@elland elland marked this pull request as ready for review August 19, 2024 09:24
@elland elland force-pushed the elland/saml-email branch from 45a6751 to 063caf0 Compare August 19, 2024 10:13
Copy link
Contributor

@mdimjasevic mdimjasevic 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 a lot of diff in golden tests. They pretty much all seem to be facing the same issue: a change in the string value where no change was needed, at least that's my hunch (e.g., replacing Email {emailLocal = "OR\32966c", emailDomain = "\RS\ENQr"} with unsafeEmailAddress "some" "example". If it's the case that the former is not a valid email address, then I can see how the change is justified, but please confirm.

Copy link
Contributor

@mdimjasevic mdimjasevic 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 stuff to improve. See comments inlined.

Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Assuming you fix the two remaining TODOs, this is good to go.

In Spar.Scim.User you still have this comment:
TODO(elland): verify with fisx if this still applies.
And another one:
TODO: validate fallback emails?

@elland elland force-pushed the elland/saml-email branch from 3b6d5dd to 68a03de Compare August 19, 2024 14:23
@elland elland force-pushed the elland/saml-email branch from f4a06a3 to 73161b1 Compare August 21, 2024 09:17
@elland elland merged commit 774f39a into develop Aug 22, 2024
9 of 10 checks passed
@elland elland deleted the elland/saml-email branch August 22, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants