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.

Sign up for GitHub

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

Jump to bottom

feat(carts): Remove cart email column #18559

Merged
merged 1 commit into from
Mar 24, 2025
Merged

feat(carts): Remove cart email column #18559

merged 1 commit into from
Mar 24, 2025

Conversation

david1alvarez
Copy link
Contributor

Because

  • The cart email is a diplication of the data from the cart's mozilla account (if it has one)

This pull request

  • Removes the email field from the carts table
  • Updates the type references
  • Provides the necessary patches

Issue that this pull request solves

Closes: FXA-11314

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@david1alvarez david1alvarez requested a review from a team as a code owner March 17, 2025 22:59
@david1alvarez david1alvarez force-pushed the FXA-11314 branch 5 times, most recently from 05310fd to 5086934 Compare March 19, 2025 00:33
StaberindeZA
StaberindeZA approved these changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

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

r+. Just the one question about the migration script.

packages/db-migrations/databases/fxa/patches/patch-163-164.sql Outdated
Comment on lines 2 to 3
-- `email` field can be populated via the `uid` column in the `accounts` table.
-- Throw an error if any carts have an email value but no uid value. Only update the patch level if successful
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? Could the column just be dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assumption as I understand it is that the cart should always have a uid if it has an email. We could simply not check for it as it shouldn't catch anything.

If it doesn't seem worthwhile, I'd be happy to drop the check

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm inclined to say it's not worth it. But maybe I'm not seeing risk here.

Assume, we hard drop the email column, regardless of if there was or wasn't a uid. What would be the risk of having a cart row which previously had an email, but doesn't have a uid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the main concern around this is that we may have carts that were previously attributed to a stub account via their email, but that didn't have an actual uid associated with them. Do you know if that's likely to be an issue? I don't recall if the stub account logic was ever actually live

@david1alvarez
feat(carts): Remove cart email column
327a953 
Because:

* The cart email is a diplication of the data from the cart's mozilla account (if it has one)

This commit:

* Removes the email field from the carts table
* Updates the type references
* Provides the necessary patches

Closes #FXA-11314
@david1alvarez david1alvarez merged commit e0df65e into main Mar 24, 2025
23 checks passed
@david1alvarez david1alvarez deleted the FXA-11314 branch March 24, 2025 21:48
@clouserw clouserw mentioned this pull request Mar 25, 2025
Merged
@clouserw
Copy link
Member

I think this will break our sync jobs. We need to be careful when removing columns. I think this patch will fix it and have asked for review from the data team: mozilla/bigquery-etl#7229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StaberindeZA StaberindeZA StaberindeZA approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@david1alvarez @clouserw @StaberindeZA