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

Add alternate accounts to the user model #1171

Merged
merged 1 commit into from
May 23, 2024
Merged

Add alternate accounts to the user model #1171

merged 1 commit into from
May 23, 2024

Conversation

jchristgit
Copy link
Member

Introduce a way to store alternate accounts on the user, and add the PATCH /bot/users/<id:str>/alts endpoint, which allows updating the user's alt accounts to the alt accounts in the request..

@jchristgit jchristgit added area: backend Related to internal functionality and utilities area: API Related to or causes API changes language: python Involves Python code type: feature New feature or request priority: 2 - normal Normal Priority level: 2 - advanced area: moderation Related to community moderation functionality: (moderation, defcon, verification) labels Dec 12, 2023
Copy link

netlify bot commented Dec 12, 2023

Deploy Preview for pydis-static failed.

Name Link
🔨 Latest commit 447ce10
🔍 Latest deploy log https://app.netlify.com/sites/pydis-static/deploys/664adc7f918b20000845d9f0

@jchristgit jchristgit force-pushed the user-alts branch 4 times, most recently from 54ab4af to 8b1c34e Compare December 12, 2023 09:33
@coveralls
Copy link

coveralls commented Dec 12, 2023

Coverage Status

coverage: 100.0%. remained the same
when pulling 447ce10 on user-alts
into 350c2b7 on main.

@jchristgit jchristgit force-pushed the user-alts branch 2 times, most recently from 74d2257 to a67f5f2 Compare December 12, 2023 12:34
Comment on lines 683 to 701
# This should be edited on a separate endpoint only
frozen_fields = ('alts',)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we making this design decision over allowing direct updates on the existing users endpoint?

Copy link
Member

Choose a reason for hiding this comment

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

As a client the bot would need to fetch the user, edit the list of alts, then send back the updated list to site.

Since that's not an atomic change, we could lose data if two actions were preformed on the same user at the same time.

This approach means that each action is preformed atomically by site

Copy link
Member

Choose a reason for hiding this comment

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

Can we not support a partial update? We only need to update the alts field, not the entire user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Strictly speaking this line doesn't do anything at all, because from my
testing DRF already doesn't allow editing the field over the regular
patch endpoint - it just ignores whatever you give it. The
frozen_fields therefore serve more of a documentary purpose than
actually changing anything. I haven't found a way to enable this in
DRF...

@jchristgit jchristgit force-pushed the user-alts branch 5 times, most recently from d296f8f to 70f5e39 Compare December 16, 2023 08:49
@jchristgit jchristgit requested a review from jb3 May 19, 2024 08:33
Copy link
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

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

All looks good, only thing I've noticed is we store user display name but it looks missing from a lot of this.

It looks like the missing serialisation here was already present but maybe it should be added in now?

pydis_site/apps/api/serializers.py Outdated Show resolved Hide resolved
@jb3
Copy link
Member

jb3 commented May 19, 2024

Oh just noticed the PR age, the serialisation might be present in main then.

Introduce a way to store alternate accounts on the user, and add the
`PATCH /bot/users/<id:str>/alts` endpoint, which allows updating the
user's alt accounts to the alt accounts in the request..
@jchristgit
Copy link
Member Author

Migrations are merged, display_name was added, please re-review.

Copy link
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

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

I may misunderstand Django migrations and this may be a non-issue, but do we need to update the dependencies of the 0093_user_alts migration to instead depend on 0095_user_display_name (which should be the most recent migration)

@jchristgit
Copy link
Member Author

@jb3

I may misunderstand Django migrations and this may be a non-issue, but do we need to update the dependencies of the 0093_user_alts migration to instead depend on 0095_user_display_name (which should be the most recent migration)

We do not. The merged migration takes care of resolving conflicts.

@ChrisLovering ChrisLovering merged commit 089b7f4 into main May 23, 2024
8 of 12 checks passed
@ChrisLovering ChrisLovering deleted the user-alts branch May 23, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Related to or causes API changes area: backend Related to internal functionality and utilities area: moderation Related to community moderation functionality: (moderation, defcon, verification) language: python Involves Python code level: 2 - advanced priority: 2 - normal Normal Priority type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants