1. OpenREM Avatar OpenREM
  2. OpenREM
  3. OpenREM
  4. Issues

All RDSR modalities need to be re-matched after upgrade to 0.9.0b6

Issue #727 resolved
Ed McDonagh created an issue

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!

Comments (29)

  1. Ed McDonagh reporter

    We use a get_or_create on the UniqueEquipmentNames table, 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?

  2. Ed McDonagh reporter

    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?

  5. Ed McDonagh reporter

    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.

  6. Ed McDonagh reporter

    That doesn't provide a solution to this issue of finding the most appropriate existing entry, but does need to be dealt with too!

  7. Ed McDonagh reporter

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

  9. Luuk

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

  10. Ed McDonagh reporter

    Thanks Luuk. I've started writing tests for this and it doesn't work yet, so don't implement this in your live systems!

  11. Luuk

    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.

  15. Luuk

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

  16. Ed McDonagh reporter

    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!

  17. Luuk

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

  18. Ed McDonagh reporter

    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!

  21. Ed McDonagh reporter

    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?

  22. Luuk

    Hi @edmcdonagh, The error will reproduce as follows (in 0.9.0b6):

    1. Import an cr/dx object --> Unique Equipment Name record is created with device_observer_uid is None/null

    2. Import an rdsr object of the same system --> Unique Equipment Name record is created with a non-empty device_observer_uid

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

  24. Luuk

    Probably because I was unclear before. Another option would be that during an rdsr-import:

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

    2. 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)

    3. 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).

  26. Ed McDonagh reporter

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

  30. Log in to comment
Assignee
–
Type
bug
Priority
critical
Status
resolved
Component
Import: RDSR
Milestone
0.9.0
Votes
0
Watchers
1
Jira: the preferred issue tracker for Bitbucket. Join the team!