Conversation
joemull
force-pushed
the
3168-ror
branch
2 times, most recently
from
d314d51 to
cc46446
Compare
|
If you enter an organization name that isn't found (e.g., in test import, "Birkbeck") and then click on one of the pagination items at the bottom, you are redirected to an erroring page eg: This errors because the item isn't found, but the search query is inserted into the pagination link |
This turned out to be a Django bug I believe. I think it's obscure because most people don't encounter it, since they submit their search query to change the list of items before selecting a page. But I've gone ahead and fixed it for this view.
Yes, good call. I added a label. I also updated the save method to make the affiliation primary if it's the first one.
That's standard, I'm pretty sure. See comment inline. |
|
Martin approved this off GitHub on 13 Dec. Moving it along to @ajrbyers. |
ajrbyers
left a comment
ajrbyers
left a comment
There was a problem hiding this comment.
This is generally very good. The CBVs are growing on me... slowly.
There are some comments inline that should be addressed. Here are some general comments in addition:
- Institution and Affiliation fields are removed without preservation.
- Mauro made a suggestion to handle setting inst/dept etc using the Manager, this should be added.
- When my affiliation was snapshotted it did not include the start/end date or the title.
- There is a conflict currently listed
mauromsl
left a comment
mauromsl
left a comment
There was a problem hiding this comment.
Mega job here @joemull
I've added a few comments regarding backwards compatibility and database integrity but very happy overall.
I'm testing out the ROR data import procedure locally to try and anticipate any possible hiccups in production but I don't want to hold you up further, good work!
| def get_or_create(self, defaults=None, **kwargs): | ||
| """ | ||
| Backwards-compatible override for affiliation-related kwargs | ||
| """ |
There was a problem hiding this comment.
we should instead do this at the Queryset level, since django will delegate this call to AccountQuerySet and the queryset also exposes these ORM operations.
Also, don't forget about get, create, filter and update_or_create
The bulk operations can be ignored IMO
There was a problem hiding this comment.
Alright, I've given this a shot with AffiliationCompatibleQueryset. Let me know what you think.
| language = models.CharField( | ||
| max_length=10, | ||
| blank=True, | ||
| choices=submission_models.LANGUAGE_CHOICES, | ||
| ) |
There was a problem hiding this comment.
Is there a rationale behind avoiding the use of django-modeltranslations here? Does it have to do with the need for a distinction between translations and ror_display_for?
There was a problem hiding this comment.
Thanks for raising this. Yes, ROR data sometimes has multiple names per language. I've written it up here: #4657
Coincidentally, the importer was bugged because ROR gives 2-character language codes but our LANGUAGE_CHOICES uses 3-character codes. So I've fixed that by using a small ISO-639 utility. I've also documented which version of ISO-639 we hardcode, for future reference.
mauromsl
left a comment
mauromsl
left a comment
There was a problem hiding this comment.
Awesome! a couple minor nitpicks and we are good to go. This is turning into a very nice piece of kit :)
| backwards_keys = ( | ||
| ( | ||
| # Account and FrozenAuthor had 'institution' | ||
| re.compile(r'^institution'), | ||
| 'controlledaffiliation__organization__labels__value', | ||
| ), | ||
| ( | ||
| # PreprintAuthor had 'affiliation' | ||
| re.compile(r'^affiliation'), | ||
| 'controlledaffiliation__organization__labels__value', | ||
| ), | ||
| ( | ||
| # Account and FrozenAuthor had 'department' | ||
| re.compile(r'^department'), | ||
| 'controlledaffiliation__department', | ||
| ), | ||
| ( | ||
| # Account and FrozenAuthor had 'country' | ||
| re.compile(r'^country'), | ||
| 'controlledaffiliation__organization__locations__country', | ||
| ) | ||
| ) |
There was a problem hiding this comment.
can these be turned into a constant / module level variable so that they don't get redefined on every call to this method
| affil_kwargs = self._pop_old_affiliation_lookups(kwargs) | ||
|
|
||
| obj = self.model(**kwargs) | ||
| obj.clean() | ||
| self._for_write = True | ||
| obj.save(force_insert=True, using=self.db) | ||
|
|
||
| if affil_kwargs: | ||
| self._create_affiliation(affil_kwargs, obj) |
There was a problem hiding this comment.
This is a bit of an anti-pattern, we need to document why we are overriding the parent behaviour without calling super, either as a comment or in the docstring
4 participants
Closes #3168.
Overview
This piece of work adds a ROR data model, including affiliations, organizations, organization names, and locations. It includes an import routine to fetch and process ROR's full database. I've included as much backwards compatibility as possible for importers that do not know about ROR. It also adds a user interface for editing one's own affiliations, and an admin interface for all of the relevant models.
Command line interface
User interface