-
Notifications
You must be signed in to change notification settings - Fork 2
[New Instrument Name] Delete Instrument Names and set umil_label
#373
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
base: develop
Are you sure you want to change the base?
Changes from all commits
4226e7b
659995a
41970f5
54e771f
009afe7
e0b4f5f
322082d
e28e9d7
9375649
1e60805
8ecc56c
d1f900f
ec8d31a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # Generated by Django 4.2.5 on 2025-08-20 15:50 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("instruments", "0010_alter_instrumentname_contributor"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RemoveConstraint( | ||
| model_name="instrumentname", | ||
| name="unique_umil_label_per_instrument_language", | ||
| ), | ||
| migrations.AddField( | ||
| model_name="instrumentname", | ||
| name="deleted", | ||
| field=models.BooleanField( | ||
| default=False, | ||
| help_text="Soft delete flag. If true, this name is considered deleted but retained in the database.", | ||
| ), | ||
| ), | ||
| migrations.AddConstraint( | ||
| model_name="instrumentname", | ||
| constraint=models.CheckConstraint( | ||
| check=models.Q( | ||
| models.Q(("umil_label", True), _negated=True), | ||
| models.Q(("verification_status", "verified"), ("deleted", False)), | ||
| _connector="OR", | ||
| ), | ||
| name="umil_label=true only if name is Verified and not Deleted", | ||
| ), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| from django.db import models | ||
| from django.db.models import Q | ||
|
|
||
|
|
||
| class InstrumentName(models.Model): | ||
|
|
@@ -34,17 +35,80 @@ class InstrumentName(models.Model): | |
| default=False, | ||
| help_text="Is this name already on Wikidata?", | ||
| ) | ||
| deleted = models.BooleanField( | ||
| default=False, | ||
| help_text="Soft delete flag. If true, this name is considered deleted but retained in the database.", | ||
| ) | ||
|
|
||
| # Custom validation to ensure at most one UMIL label per instrument language | ||
| # Constrain umil_label to be true only if the name is verified and not deleted | ||
| class Meta: | ||
| constraints = [ | ||
| models.UniqueConstraint( | ||
| fields=["instrument", "language"], | ||
| condition=models.Q(umil_label=True), | ||
| name="unique_umil_label_per_instrument_language", | ||
| ) | ||
| models.CheckConstraint( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So are you getting rid of the UniqueConstraint?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that with my reassignment logic, where if an admin sets
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean by "reassignment logic"? I think this would just require a different order (set
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The wall I've run into here regarding the Unique Constraint is that partial unique constraints (unique constraints with a condition) cannot be deferred. I thought I could get around this by deferring the Unique Constraint (https://docs.djangoproject.com/en/5.2/ref/models/constraints/#deferrable), so that the constraint is not enforced until the end of the transaction. Otherwise the constraint is immediate and enforced after every command. The solution to this is implementing a check in the custom save function where a ValueError is raised if we detect more than one
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: looking into the difference between the save_model() in admin vs. save() in the view... |
||
| name="umil_label=true only if name is Verified and not Deleted", | ||
| check=~Q(umil_label=True) | ||
| | (Q(verification_status="verified") & Q(deleted=False)), | ||
| ), | ||
| ] | ||
|
|
||
| # TODO: add verified_by field to track who verified the name | ||
| def __str__(self): | ||
| return f"{self.name} ({self.language.en_label}) - {self.instrument.wikidata_id}" | ||
|
|
||
| def save(self, *args, **kwargs): | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't we give the user/superuser a choice in this case? Like alert them that they are deleting an instrument label and give them a choice of selecting another one?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since a user can only delete their own name entries, In the current implementation when What could be more informative to an admin is to:
If a user deletes a label from the front end, the page will have to exist with "No verified label in [language]", until a new label is assigned manually (even if there are other verified names in that language displayed/available). I think that the best final option to maintain clarity to the admin is to keep my current automatic reassignment logic, but provide a warning messages for unassignment and reassignment of the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think I was confused about the purpose of this a bit. So, it seems like the only case where we need any custom "save" logic is when the object in question is trying to save with a change in Is that right? If so, we can exit early from this function in all other cases. I'm less concerned about giving a choice because I reviewed the code for adding a name and it looks like we randomly assign a Since this process involves multiple database operations/modifications, you will need to think about database consistency and atomicity (what happens if one of the database operations fails? is your database ever in an invalid state?) You will need to use database transactions: https://docs.djangoproject.com/en/5.2/topics/db/transactions/.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I see what you mean ! I can check if the in memory status of the name regarding I'm a bit confused by what you mean regarding adding a name, because another change to this process is that So to summarize, I will modify my save function so that it only considers:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, good point. Makes sense. |
||
| existing_umil_label_qs = InstrumentName.objects.filter( | ||
| instrument=self.instrument, | ||
| language=self.language, | ||
| umil_label=True, | ||
| ).exclude(id=self.id) | ||
|
|
||
| existing_umil_label = existing_umil_label_qs.first() | ||
| print("Existing UMIL label:", existing_umil_label) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of this print statement in a deployed setting? Was this just for debugging in development?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry ! Forgot to delete this, was part of debugging ! |
||
|
|
||
| replacement_umil_label = ( | ||
| InstrumentName.objects.filter( | ||
| instrument=self.instrument, | ||
| language=self.language, | ||
| verification_status="verified", | ||
| deleted=False, | ||
| ) | ||
| .exclude(id=self.id) | ||
| .order_by("id") | ||
| .first() | ||
| ) | ||
| print("Replacement UMIL label:", replacement_umil_label) | ||
|
|
||
| # If setting umil_label=True | ||
| if self.umil_label: | ||
| if self.deleted: | ||
| # Assign replacement if possible, else just unset | ||
| self.umil_label = False | ||
| if replacement_umil_label: | ||
| replacement_umil_label.umil_label = True | ||
| replacement_umil_label.save() | ||
|
|
||
| return super().save(*args, **kwargs) | ||
| else: | ||
| # Unset other umil_labels in one query | ||
| existing_umil_label_qs.update(umil_label=False) | ||
|
|
||
| # If a verified name is removing itself as umil_label | ||
| if not self.umil_label: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be an |
||
| # check if there is an existing label | ||
| if existing_umil_label: | ||
| pass | ||
| # If the existing label is not found, check for a replacement | ||
| elif replacement_umil_label: | ||
| print("Replacement label found.", flush=True) | ||
| replacement_umil_label.umil_label = True | ||
| replacement_umil_label.save() | ||
| # If there is no replacement umil_label, only set umil_label=False if the current name is also deleted | ||
| else: | ||
| # Otherwise, a viewer will see a verified name on the detail page without a label | ||
| if self.verification_status == "verified" and not self.deleted: | ||
| # raise ValueError( | ||
| # "This is the only verified, non-deleted name for this instrument and language, it must be set as umil_label." | ||
| # ) | ||
| self.umil_label = True # Comment this out and uncomment the above after initial instrument upload. | ||
|
|
||
| super().save(*args, **kwargs) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on this function is now out of date.