[New Instrument Name] Delete Instrument Names and set `umil_label` #373

mailynmailyn · 2025-08-21T16:15:28Z

refs:

- indicates a name has been deleted but preserves in the db refs: #340

…ment name from UMIL - allow admin to filter instrument names by deletion status - change deletion UI message to better reflect that the name will persist on the the UMILdb - only display instrument names that are `deleted`=`false` - create migration file refs: #340

- change constraint: `umil_label` = `true` only when verified and not deleted - existing_umil_label: other instrument_name in language where umil_label=true - replacement_umil_label: first verified, non-deleted instrument name added in language - `umil_label` set to `true`: unset `existing_umil_label` -`umil_label` set to `false`: reassign if possible to `replacement_umil_label` -`umil_label` set to `true` and `deleted` = `true`: indicates that name has been deleted from frontend --> reassign if possible, otherwise there are no verified instrument names refs: #350

…ruments will never be verified refs:#350

… where `umil_label` = `true` refs:#350

…ument names for the label in a language refs:#350

…ion must set `umil_label`=`true` refs:#350

- make use of the update function and querysets - eliminate recusivity - specify constraint name refs:#350

- otherwise update would set all other verified, non-deleted names as umil_label - order by id to get the correct first added name - consider scenario where the replacement_umil_label IS the current umil_label: avoid redundant resetting - remove unique umil_label constraint, the process of swapping labels will violate the constraint (seemingly the constraints are checked ebfore the custom save logic) refs:#350

- consider constraint migration refs:#350

- replacement_umil_label can be the existing_umil_label (umil_label=true) - prevents reassignment of a current umil_label - check for existing label when umil_label is "unset" to false refs: #350

mailynmailyn · 2025-08-22T14:48:42Z

@kunfang98927 this is an interesting case:
When I attempted to import_instruments with this branch, my umil_label assignment code raised an error indicating that a name was being saved as umil_label=false when it is the only verified name available.
I looked up to see what was causing this, and looks like sometime there are cases on Wikidata when there is only an alias and no label !

If an instrument name is verified and not deleted, it must have umil_label=true, otherwise a viewer/contributor will see a verified name but no label. But then the umil_label will be assigned to an alias on Wikidata.
Maybe we need to specify a wiki_label attribute ?

Instead of returning a ValueError, I can ensure that the alias is assigned as the label on UMIL ?
This probably will require greater discussion to figure out.

refs:#350

…deleted attribute refs:#350

annamorphism · 2025-08-22T18:15:44Z

@kunfang98927 this is an interesting case: When I attempted to import_instruments with this branch, my umil_label assignment code raised an error indicating that a name was being saved as umil_label=false when it is the only verified name available. I looked up to see what was causing this, and looks like sometime there are cases on Wikidata when there is only an alias and no label !

I imagine there could be more cases like this. Even when making new items, the restriction is to have at least a label, an alias, or a description, but there's no requirement to have all three (and that's not even getting into people adding to an extant item with an alias rather than a label, or removing an incorrect label but leaving a correct alias, etc...)

annamorphism · 2025-08-22T18:27:23Z

fwiw it looks like this particular case was due to a single edit last November https://www.wikidata.org/w/index.php?title=Q193666&diff=prev&oldid=2272030511 where somebody changed the labels in a whole bunch of languages--sometimes to clean up capitalization, but in other cases for no obvious reason (like Hebrew or Azerbaijani, for which there is a corresponding wikipedia article under the old label but no label on the wikidata item.)

dchiller · 2025-08-24T22:43:18Z

web-app/django/VIM/apps/instruments/admin.py

                "instrument",
                "language",
                "name",
                "source_name",


Comment on this function is now out of date.

dchiller · 2025-08-24T22:44:42Z

web-app/django/VIM/apps/instruments/models/instrument_name.py

-                condition=models.Q(umil_label=True),
-                name="unique_umil_label_per_instrument_language",
-            )
+            models.CheckConstraint(


So are you getting rid of the UniqueConstraint?

It seems that with my reassignment logic, where if an admin sets umil_label=true on a name and then the existing umil_label name is set as false, there is a brief point in time where there are two names with umil_label=true which is prevented by this Constraint.

I'm not sure what you mean by "reassignment logic"? I think this would just require a different order (set umil_label = False on existing label name and then umil_label = True on the new one)

The wall I've run into here regarding the Unique Constraint is that partial unique constraints (unique constraints with a condition) cannot be deferred.
The issue is that if we enforce the Unique constraint of AT MOST one umil_label per instrument language, this is always broken when an admin sets another name's umil_label=true regardless if in the custom save function swaps the existing umil_label before calling the super().save

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.
With a bit of debugging it seems like when I click save on the django admin interface for an instrument name object, we do not seem to ever enter the custom save function so I think that although the state is not written to the database, there is still some sort of updating of the object that triggers the constraint.
I'm not finding much documentation specifically on partial unique constraints and deferral but after I compose up, the app fails to build with a Value Error indicating that partial unique constraints cannot be deferred.

The solution to this is implementing a check in the custom save function where a ValueError is raised if we detect more than one umil_label for names in the instrument language, evaluated after a potential swap/reassignment happens. But this will require the removal of the Unique Constraint.

Update: looking into the difference between the save_model() in admin vs. save() in the view...
Assuming they must be different processes because the delete_name function is able to bypass the Check Constraint (when a contributer deletes a name on the frontend)

dchiller · 2025-08-24T22:45:47Z

web-app/django/VIM/apps/instruments/models/instrument_name.py

+        ).exclude(id=self.id)
+
+        existing_umil_label = existing_umil_label_qs.first()
+        print("Existing UMIL label:", existing_umil_label)


What is the purpose of this print statement in a deployed setting? Was this just for debugging in development?

Sorry ! Forgot to delete this, was part of debugging !

dchiller · 2025-08-24T22:48:16Z

web-app/django/VIM/apps/instruments/models/instrument_name.py

+                existing_umil_label_qs.update(umil_label=False)
+
+        # If a verified name is removing itself as umil_label
+        if not self.umil_label:


This could be an else statement following from line 82 right?

dchiller · 2025-08-24T22:50:20Z

web-app/django/VIM/apps/instruments/models/instrument_name.py

        return f"{self.name} ({self.language.en_label}) - {self.instrument.wikidata_id}"
+
+    def save(self, *args, **kwargs):
+


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?

Since a user can only delete their own name entries, umil_label reassignment would only be done on the admin interface page.
We can display a warning message on the django admin page that the admin is deleting the label and should choose another, however it opens the possibility that no verified instrument names are assigned as the label, which doesn't make sense on the frontend (visible verified names, but "no label available").
I think that Unique Constraint only ensures that "at most" one object with umil_label=true, which does not solve this problem (but then also it would be impossible to choose a different label)

In the current implementation when umil_label is unassigned, the custom save function will reassign umil_label to the earliest available verified, non-deleted name. If an admin assigns a name as the umil_label, it will unset the existing label. This ensures that there is always ONE umil_label at all times if possible.

What could be more informative to an admin is to:

reinstate the unique constraint

maintain the current check constraint to prevent impossible states

if an admin unassigns a umil_label, provide a warning message but allow

if an admin wants to reassign the umil_label, they must unassign the current umil_label (unique constraint)
The only downside to this is the case mentioned above (no automatic reassignment)

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 umil_label, where an admin it notified what name gained or lost umil_label=true respectively.

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 umil_label from its previous value or if the object is being saved with umil_label = True and deleted = True.

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 umil_label to initial names, so I think this is fine.

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/.

I think I see what you mean ! I can check if the in memory status of the name regarding umil_label does not match the database status, which will trigger the custom save logic.
In addition to the case of umil_label = True and deleted = True, I think a final case to consider would be when the first non-deleted instrument name is verified (by a reviewer). In this case, the reviewer must be forced to save the instrument name as the umil_label.

I'm a bit confused by what you mean regarding adding a name, because another change to this process is that umil_label will always be False with a new name because it is unverified.
This opens another possible case of an admin adding an instrument name on the backend, and setting it as the umil_label, which would not be considered as the label status changing. However, I think that this can be flagged by the Unique Constraint, which I will try to reinstate with what you pointed out in here.

So to summarize, I will modify my save function so that it only considers:

umil_label has changed

umil_label=True and deleted=True (contributor has deleted the label from the frontend)

umil_label=False, verification_status=True and deleted=False, and there is no other verified name entry

and reinstate the Unique Constraint so that it prevents an admin from adding an instrument name as umil_label when there is an existing umil_label for that instrument language.

I'm a bit confused by what you mean regarding adding a name, because another change to this process is that umil_label will always be False with a new name because it is unverified.

Ah, good point.

Makes sense.

yinanazhou · 2025-09-17T19:38:39Z

This PR has been hanging for a while. What is the current status?

mailynmailyn · 2025-09-17T23:13:13Z

This PR has been hanging for a while. What is the current status?

Still in progress... will push some changes regarding #350 but if there are still problems, #350 might need deeper discussion and this PR can be closed.

kyrieb-ekat · 2026-01-07T20:59:31Z

Just wanted to bump this; @kunfang98927 is this still something needed?

mailynmailyn added 11 commits August 20, 2025 11:34

feat: create deleted boolean flag for instrument name

4226e7b

- indicates a name has been deleted but preserves in the db refs: #340

chore: remove functinality to assign umil_label as newly added inst…

54e771f

…ruments will never be verified refs:#350

refactor: the active_instrument_label is always set to the instrument…

009afe7

… where `umil_label` = `true` refs:#350

feat: notify user on the detail page that there are no verified instr…

e0b4f5f

…ument names for the label in a language refs:#350

refactor: raise error that notifies the reviewer that their verificat…

322082d

…ion must set `umil_label`=`true` refs:#350

refactor: reduce queries of custom save function

e28e9d7

- make use of the update function and querysets - eliminate recusivity - specify constraint name refs:#350

fix: alter migration file post rebase

1e60805

- consider constraint migration refs:#350

fix: logic bugs

8ecc56c

- replacement_umil_label can be the existing_umil_label (umil_label=true) - prevents reassignment of a current umil_label - check for existing label when umil_label is "unset" to false refs: #350

mailynmailyn requested review from dchiller and kunfang98927 August 21, 2025 16:15

mailynmailyn marked this pull request as ready for review August 21, 2025 17:15

mailynmailyn added 2 commits August 22, 2025 12:59

fix: save Wikidata alias as label if there is no Wikdiata label

d1f900f

refs:#350

fix: allow reviewer to modify umil_label attribute, remove access to …

ec8d31a

…deleted attribute refs:#350

annamorphism mentioned this pull request Aug 22, 2025

Establish a Hornbostel Sachs three level flowchart/decision tree for new instrument additions. #337

Open

dchiller reviewed Aug 24, 2025

View reviewed changes

		return f"{self.name} ({self.language.en_label}) - {self.instrument.wikidata_id}"

		def save(self, args, *kwargs):

[New Instrument Name] Delete Instrument Names and set umil_label #373

Are you sure you want to change the base?

[New Instrument Name] Delete Instrument Names and set umil_label #373

Uh oh!

Conversation

mailynmailyn commented Aug 21, 2025

Uh oh!

mailynmailyn commented Aug 22, 2025

Uh oh!

annamorphism commented Aug 22, 2025

Uh oh!

annamorphism commented Aug 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dchiller Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yinanazhou commented Sep 17, 2025

Uh oh!

mailynmailyn commented Sep 17, 2025

Uh oh!

kyrieb-ekat commented Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[New Instrument Name] Delete Instrument Names and set `umil_label` #373

[New Instrument Name] Delete Instrument Names and set `umil_label` #373

dchiller Aug 28, 2025 •

edited

Loading