With match-on-UID on or off, RDSRs are not matched the first time they are imported after an upgrade to 0.9.0b6.
Confirmed by @dplatten with match-on-UID off, and @edmcdonagh with match-on-uid on.
Need to fix or document for final release. Preferably the former!
We might also want to consider if the device_observer_uid should be part of the uniqueness, though I suspect it isn't the RDSRs we normally have problems distinguishing between?
We can use the second element in the tuple get_or_create returns to determine if the object was created, and then act accordingly to tidy up if the only change was the addition of device_observer_uid
https://simpleisbetterthancomplex.com/tips/2016/07/14/django-tip-6-get-or-create.html
Doh! Now realise that part of the tuple is already being used...
Looking at unique_together, I think we probably need to add a hash of the device_observer_uid. Else we could have a situation where two devices are set up identically in all fields except device_observer_uid, and Django would refuse to create the new entry because it can't have a new entry that wasn't unique, which it wouldn't be according to the current list.
That doesn't provide a solution to this issue of finding the most appropriate existing entry, but does need to be dealt with too!
Added device_observer_uid_hash, added to unique_together. Added test for same except device_observer_uid, apply last
display name found for same unit. Refs #727
→ <<cset 15d3ec81b348>>
@dplatten @LuukO - what do you think of 15d3ec81b348 as a solution?
I performed the updating of the display names happily, thinking this was the last time I ever needed to do that. But I didn't realise that it was also an issue if the option was turned off. I think it is a good solution. A small note. I actually would make line 1127 and 1142 the same (now one could think why they are different):
if not equip_display_name.display_name:
Btw. Since 0.9.0b6 I have a number of "failed imports" in my radiography table. I never had them, so this might be related. But I still have to look at it (will do that today).
Thanks Luuk. I've started writing tests for this and it doesn't work yet, so don't implement this in your live systems!
So I found the issue. If you have an already existing modality without device_observer_uid (pre 0.9.0b6 version) and in version 0.9.0b6 you import an rdsr of this device it works fine. But if you import from the same device an "image" object after the rdsr was imported, the get_or_create() function in (e.g.) dx.py fails as 2 Unieque Equipment Name objects are returned. One with and one without the device_observer_uid, but get_or_create doesn't see the difference.
@edmcdonagh do you want to take this issue into account in this issue or shall I create a new issue.
I think I'm handling that by adding device_observer_id_hash to unique_together
Ok, I will be able to test that easily.
Added test for upgrade situation and corrected rdsr code as a result. Hoorah for testing. Refs #727
→ <<cset faa66876054e>>
@edmcdonagh Thanks for the update. Regretfully that didn't solve the issue as described above by me. What helps is to add in the get_or_create statement in dx.py (and probably we should do that also in the other extractors except for rdsr.py):
device_observer_uid_hash=hash_id('')
In the get_or_create() statement (line 476). I actually don't know if the hash of an empty string is also an empty value. If that is the case the hash of the empty string can be replaced by the empty string itself.
Sorry, I think I solved a related problem for rdsr imports, but I haven't dealt with the other extractor routines.
I'll get into it later if that's ok?
Thanks for checking and pointing it out!
That's fine for me. But I thought that you thought it should work as you stated: "I think I'm handling that by adding device_observer_id_hash to unique_together".
That sorts out situation where two systems, identical except for the device observer UID would cause an error, because get_or_create would want to have a new row, but as both would be identical according to unique_together this would not be allowed. By adding a device observer UID hash to unique_together, this should work fine.
What I haven't considered to this point is what happens with the other modalities - I've probably fixed one problem and created two more!
Reformat, and commented out code for device_observer_uid, possibly not necessary... Refs #727
→ <<cset 72fa42de52ec>>
Test for importing images without fixing device_observer_uid - expected to fail, doesn't. Refs #727
→ <<cset b545e194d8a3>>
Hi @LuukO. I've been trying to create the fault condition so I can get it to fail a test, then update the code so it passes. But I've not managed it!
What I was trying is to import an image, set the device_observer_uid and device_observer_uid_hash to None then importing again, as per b545e19.
I had expected to 'fix' it with the code I have added (commented) to dx.py as per 72fa42d.
However, hash_id('') == hash_id(None) == None, so I don't think there will be a problem with unique_together. But, maybe it is different when the the field is created by a migration. Who knows?
Can I check - did you do the migration before testing this?
Obviously we can just put the code in, but I don't think it will do anything!
What do you think?
Hi @edmcdonagh, The error will reproduce as follows (in 0.9.0b6):
Import an cr/dx object --> Unique Equipment Name record is created with device_observer_uid is None/null
Import an rdsr object of the same system --> Unique Equipment Name record is created with a non-empty device_observer_uid
Import an cr/dx object again of this system --> Error is thrown as 2 Unique Equipment Name records match the filter (without filtering on device_observer_uid): "remapp.models.MultipleObjectsReturned: get() returned more than one UniqueEquipmentNames -- it returned 2!"
I renamed the display after step 1 and 2, but I don't think that is necessary to reproduce the error.
To solve this error, one needs to add an empty device_observer_uid in get_or_create() while importing a non-rdsr object. Then it will only match on the Unique Equipment Name record with the empty device_observer_uid (so you will get only one result while filtering).
I hope it is clear now how to reproduce the error and what is the cause of the error.
Yes. I think you might have finally got through! Sorry! I'll write up the tests now.
Probably because I was unclear before. Another option would be that during an rdsr-import:
If a match is found based on device_observer_uid (and option to match on this is set) --> if other parameters are not the same, set display name and modality and add new record to UniqueEquipmentName, otherwise use matching record.
If a match is found based on uniqueness without device_observer_uid --> update the matching record with device_observer_uid (instead of creating a new one)
Otherwise --> Create a new UniqueEquimpentName record with autogenerated display name.
In this way the other importers don't have to be changed (as they only look at matching without device_observer_uid).
Hooray! Finally reproduced the error!. Refs #727
→ <<cset 23d364316546>>
Fix for dx.py, need to repeat for others. Supplemented tests to show how display names will be assigned depending on order of import. Refs #727
→ <<cset c9bc004ed38b>>
Fix for mam.py and ct_philips.py. Refs #727
→ <<cset 5b83a05c8107>>
Updated changes for ref #727
→ <<cset e0b037d97167>>
Merged in issue727matchonuidissues (pull request #280)
Fixes #727
Approved-by: Luuk
→ <<cset 051678319eb8>>
We use a
get_or_createon theUniqueEquipmentNamestable, and as we have added an extra field,device_observer_uid, I think it will always create a new entry.I guess we could add some code to check if we have created anew entry then see if there is an identical one with a blank observer uid and copy the name for that?