Closed zmousm closed 4 years ago
Hi @zmousm , thanks a lot for this work!
I'm just about to go on Christmas leave (for southern hemisphere/New Zealand, these are the main summer holidays) and I'll be back on deck from January 13th.
I'll see if I manage to review anything in the last ~ hour I have in the office, but I'm afraid in won't be much - I promise to look at this once I'm back in the week of January 13th.
Cheers, Vlad
No worries @vladimir-mencl-eresearch, happy holidays!
Hi @zmousm ,
Great job. I've just gone through all the commits. I'll comment inline, but it's mostly requests for clarification / more detailed documentation - no real issues discovered, looks good to me.
Cheers, Vlad
I am now pushing the second round of commits (hopefully there won't be a third round).
The first 12 commits (prefixed with fixup!) are meant to be squashed into their preceding counterparts before merging. I am not doing this now so as to not confuse you and not force you to review everything all over again.
@vladimir-mencl-eresearch, @c00kiemon5ter, @ioparaskev please review.
I hope to find some time to update the high-level overview of the changes in the linked issue.
I pushed 4 more commits to fix errata
I should also note that I have not effectively tested parse_institution_xml
as of yet...
I am now pushing the final round of fixes, after testing parse_institution_xml
.
Thank you for your efforts, I understand testing and/or reviewing is not easy for this particularly large change-set.
I will squash all fixup! commits and merge this shortly due to (external) time constraints.
Hi @zmousm ,
Thanks for all the effort put into it - and sorry about the slow action on my side. I've been trying to go through the second batch of commits, but was not progressing much - and there was a lot to follow, sometimes out of my depth with Django quirks, sometimes just plain lots of changes. But overall, it all looked reasonable.
If you'd like me to at least help testing, please let me know when ready to go (and which branch).
Cheers, Vlad
Hi @vladimir-mencl-eresearch,
throughout this effort I tried to only touch code that had to be updated for EDB v2, yet avoid working around code that needed refactoring. DjNRO is an 8 year old project, so the latter was all over the place :) I am sorry if I made it harder for you.
What you can do is deploy the code in master (considering the notes/caveats provided previously) and check if it works out for any niche use cases you may have in mind -- maybe you will find a bug I didn't catch!
Implement #62
WIP. Models and migrations are mostly done. Still pending forms, views, templates management commands.Now complete.Important notes, caveats:
UUID
is enforced forInstitution.instid
,ServiceLoc.locationid
. Such fields are not editable through forms, whileparse_institution_xml
defaults to deriving aUUID
by means of MD5 hashing, if the input is unsuitable.Coordinates
are modeled as m2m forServiceLoc
, but only a single set is supported currently. This is enforced by a signal receiver. Longitude, latitudeServiceLoc
cached properties work as a proxy, together with a related mechanism inServiceLocForm
. On the other hand,parse_institution_xml
retains the first set of coordinates it encounters in a location.Coordinates
are currently ignored in institution edit form, nor are they otherwise presented anywhere, while they are processed byparse_institution_xml
as expected.ServiceLoc.wired
toServiceLoc.wired_no
is driven bysettings.SERVICELOC_DERIVE_WIRED_NO
, which defaults to mappingTrue
to 42.parse_institution_xml
. For the latter the version may be provided through a command-line option. For the former the requested version is inferred by considering path, query parameter or resource (realm vs. ro). If no version is specified/hinted, a default is used, which, as of now, is version 2.@vladimir-mencl-eresearch,@c00kiemon5ter, @ioparaskev please start reviewing