Add Wikimedia Nick Name field #582
Conversation
Reviewer's Guide by SourceryThis PR introduces the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Gagan-Ram - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a unique constraint to the
wikimedia_usernamefield to prevent multiple users from having the same username. - The migration file includes changes to all models, but it should only include changes related to the
Usermodel.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| name='wikimedia_username', | ||
| field=models.CharField(max_length=255, null=True), | ||
| ), | ||
| migrations.AlterField( |
There was a problem hiding this comment.
issue (complexity): Consider using a loop to generate the AlterField operations for each model name to reduce repetition and improve maintainability of the migration file.
You can reduce this repetition by listing all affected model names and generating the corresponding operations in a loop. For example, instead of manually writing a block for each model, you could write:
# Generated by Django 5.1.7 on 2025-03-15 09:17
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('pretixbase', '0008_cartposition_job_title_orderposition_job_title'),
]
def generate_alter_field_operations():
model_names = [
'cachedcombinedticket', 'cachedticket', 'cancellationrequest', 'cartposition', 'checkin',
'checkinlist', 'device', 'event', 'event_settingsstore', 'eventmetaproperty', 'eventmetavalue',
'gate', 'giftcard', 'giftcardacceptance', 'giftcardtransaction', 'globalsettingsobject_settingsstore',
'invoice', 'invoiceaddress', 'invoiceline', 'item', 'itemaddon', 'itembundle', 'itemcategory',
'itemmetaproperty', 'itemmetavalue', 'itemvariation', 'logentry', 'notificationsetting', 'order',
'orderfee', 'orderpayment', 'orderposition', 'orderrefund', 'organizer', 'organizer_settingsstore',
'question', 'questionanswer', 'questionoption', 'quota', 'requiredaction', 'revokedticketsecret',
'seat', 'seatcategorymapping', 'seatingplan', 'staffsession', 'staffsessionauditlog', 'subevent',
'subeventitem', 'subeventitemvariation', 'subeventmetavalue', 'taxrule', 'team', 'teamapitoken',
'teaminvite', 'u2fdevice', 'user', 'voucher', 'waitinglistentry', 'webauthndevice'
]
return [
migrations.AlterField(
model_name=model_name,
name='id',
field=models.BigAutoField(auto_created=True, primary_key=True, serialize=False),
)
for model_name in model_names
]
operations = [
migrations.AddField(
model_name='user',
name='wikimedia_username',
field=models.CharField(max_length=255, null=True),
),
] + generate_alter_field_operations()This refactoring retains all functionality while cutting down on redundancy and improving maintainability.
There was a problem hiding this comment.
- We don't let user to modify the wikimedia_username so not adding unique constraint should be okay.
- The migration files are autogenerated when I ran python manage.py makemigrations command
There was a problem hiding this comment.
Okay, I understand.
Regarding your points:
-
wikimedia_usernameand unique constraint: You're right, since thewikimedia_usernameis not intended to be modified by the user and is populated automatically upon login via Wikimedia, adding a unique constraint might not be necessary and could even cause issues if, for some reason, the same username is associated with multiple accounts in your system (although your code prevents multiple logins). -
Autogenerated migration files: I understand that the migration files are autogenerated. My suggestion was aimed at improving the structure and maintainability of the generated code. While the code works as is, refactoring the migration to reduce repetition can make it easier to understand and modify in the future. However, if the autogeneration process is likely to overwrite manual changes frequently, then my suggestion might not be practical.
Given that the migration files are autogenerated, and the wikimedia_username field doesn't need a unique constraint, it's perfectly reasonable to keep the migration as is. My suggestion was more of a "nice-to-have" rather than a critical change.
|
@hongquan could you please look into this |
|
@Gagan-Ram Please delete all DB migration You said that they are generated when you ran
We need time to check and determine which is the case. In the meantime, to prevent the codebase from being broken if the cause is the 1st case, you need to delete those lines from this PR. |
Gagan-Ram
force-pushed
the
feature/558-add-nickname-field
branch
from
3caeb84 to
cad6020
Compare
|
@hongquan I noticed that Django is automatically applying BigAutoField because of DEFAULT_AUTO_FIELD = 'django.db.models.BigAutoField' in settings.py. |
|
Of course we know that Django applies |
|
It seems like we still need to figure out the problem. |
|
Hi, I added a comment yesterday but forgot to submit. Please check. |
Gagan-Ram
force-pushed
the
feature/558-add-nickname-field
branch
from
cad6020 to
b48226d
Compare
This PR updates the user model to include the wikimedia_username field.
Fixes: #558
Changes Made
wikimedia_usernamefield to the user model.Potential Issues / Considerations
Screenshot after the update
Summary by Sourcery
Adds a
wikimedia_usernamefield to the user model and displays it as a read-only "Nick name" field in the account settings UI. The field is automatically populated when a user logs in via Wikimedia.Enhancements: