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

Ensure user email is always in email address table #13296

Conversation

ivanvc
Copy link
Contributor

@ivanvc ivanvc commented Oct 24, 2020

This PR solves the issue with duplicated emails reported in #11251, by ensuring that the primary user email address is in the email_address table too.

Fixes #11251.

In order to avoid email duplication, ensure that once an user is created
or updated his new email address is added to the email address table.
@6543 6543 added the type/bug label Oct 24, 2020
@6543 6543 added this to the 1.14.0 milestone Oct 24, 2020
@lafriks
Copy link
Member

lafriks commented Oct 25, 2020

I don't think this can be backported anymore as there is migration

…s-stored-in-email-address-table

* origin/master:
  [UI] Hide consecutive additions and removals of the same label (go-gitea#13315)
  [skip ci] Updated translations via Crowdin
  Fix send mail (go-gitea#13312)
  [skip ci] Updated translations via Crowdin
  Deny wrong pull (go-gitea#13308)
  Group Label Changed Comments in timeline (go-gitea#13304)
  [skip ci] Updated translations via Crowdin
  Attempt to handle unready PR in tests (go-gitea#13305)
  go-gitea#12897 - add mastodon provider (go-gitea#13293)
  [skip ci] Updated translations via Crowdin
  Fix Storage mapping (go-gitea#13297)
  Update Mirror IsEmpty status on synchronize (go-gitea#13185)
  Fix bug isEnd detection on getIssues/getPullRequests (go-gitea#13299)
  systemd service: Add commented PATH environment option for Git prefix (go-gitea#13170)
  Sendmail command (go-gitea#13079)
  Various UI and arc-green fixes (go-gitea#13291)
…s-stored-in-email-address-table

* origin/master:
  Remove obsolete change of email on profile page
  Fully remove fomantic-ui from frontend build dependencies
  Fix command-line doc examples
This reverts commit 77cd657.
Update user email addresses as they are being used by other fixtures in
the email_address.yml file, belonging to user1 and user2. And used in
several tests, expecting to belong to them.
@6543
Copy link
Member

6543 commented Oct 29, 2020

I don't think this can be backported anymore as there is migration

YES we can NOT backport migration, but this should go into doctor as task and the bugfix itselfe can be backported :)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 29, 2020
@ivanvc
Copy link
Contributor Author

ivanvc commented Oct 29, 2020

Finally, I was able to fix the fixtures that were causing errors in some unit + integration tests.

It does need the migration, else users may be able to duplicate their email addresses.

@@ -186,14 +186,14 @@
lower_name: user11
name: user11
full_name: User Eleven
email: user11@example.com
email: user011@example.com
Copy link
Member

Choose a reason for hiding this comment

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

why change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in the email_1ddress.yml fixtures, the user11@example.com is registered to user1, and changing that file rather than this, brings more issues and places where it should be updated (I initially went through that route with commits bdd4bc2 and 77cd657), and decided later that the minimal impact would be to update the email address here.

Comment on lines 26 to 28
if err := x.Sync2(new(User), new(EmailAddress)); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary, because you haven't changeed the struct of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, my bad, just read some other migrations and I thought it was required.

@a1012112796
Copy link
Member

In my opion, maybe it's not necessary to kept Email cols in the user, get it from EmailAddress table when needed is enough.

@ivanvc
Copy link
Contributor Author

ivanvc commented Nov 3, 2020

In my opion, maybe it's not necessary to kept Email cols in the user, get it from EmailAddress table when needed is enough.

That was my original intention, as I removed the Email field from the User, havoc broke, as it is used in so many places that I reverted to better sync the fields in the user and email tables.

This PR has the minimum to fix the issue, if a complete re-write of the User model is what is intended (or best), I think this should be closed then, as it will be a way bigger effort.

…s-stored-in-email-address-table

* origin/master: (42 commits)
  [skip ci] Updated translations via Crowdin
  Fix bug on release publisherid migrations
  [skip ci] Updated translations via Crowdin
  Fixed git args duplication
  [skip ci] Updated translations via Crowdin
  [Vendor] update macaron related
  Add the tag list page to the release page
  fix docker rootless manifest
  Refactor image paste code
  [skip ci] Updated translations via Crowdin
  Fix 'add code comment' button being invisible all the time
  Fix reactions on code comments
  Remove specific indexer path
  [skip ci] Updated translations via Crowdin
  Misc UI fixes, add secondary color
  Set auto-tag to false on rootless manifest
  Fix typo
  docker: rootless image
  don't append key file if asked not to
  Comment box tweaks and SVG dropdown triangles
  ...
@lunny
Copy link
Member

lunny commented Feb 18, 2021

Please resolve the conflicts.

@ivanvc
Copy link
Contributor Author

ivanvc commented Feb 18, 2021

I'm not sure if actually solving the conflicts makes sense anymore. Probably, as it is very stale, it makes more sense to actually re-work it. I'm gonna close the PR and will open a new one later, whenever I get some free time.

@ivanvc ivanvc closed this Feb 18, 2021
@a1012112796 a1012112796 removed this from the 1.14.0 milestone Feb 18, 2021
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email address might be stored twice in database
6 participants