-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for the Research Organization Registry (ROR) #4483
base: master
Are you sure you want to change the base?
Conversation
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: http://127.0.0.1:8000/JRNL/profile/organization/search/?q=birkbeck&page=3&paginate_by=25 This errors because the item isn't found, but the search query is inserted into the pagination link |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks really good. Just a few small things.
- Pagination in ROR search currently redirects to page not found error
- List of affiliations should show which is primary?
- RORImportStatus should not be duplicated?
@@ -868,7 +868,7 @@ def check_for_bad_login_attempts(request): | |||
time = timezone.now() - timedelta(minutes=10) | |||
|
|||
attempts = models.LoginAttempt.objects.filter(user_agent=user_agent, ip_address=ip_address, timestamp__gte=time) | |||
print(time, attempts.count()) | |||
logger.info(time, attempts.count()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this message be more explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes probably, though I'm just removing the noisy print statement here, not changing this feature on this branch.
'is_active': True, | ||
'password': 'this_is_a_password', | ||
'salutation': 'Prof.', | ||
'first_name': 'Martin', | ||
'middle_name': '', | ||
'last_name': 'Eve', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top test author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without a doubt!
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
('started', models.DateTimeField(auto_now_add=True)), | ||
('stopped', models.DateTimeField(blank=True, null=True)), | ||
('status', models.CharField(choices=[('ongoing', 'Ongoing'), ('unnecessary', 'Unnecessary'), ('successful', 'Successful'), ('failed', 'Failed')], default='ongoing')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these choices reference RORImportStatus rather than duplicating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the standard way Django records choices in migrations, even when the choices are expressed in a class. The reason is that if the migration referenced a variable in the models file, then if the choices list ever changed there, the migration might stop working.
Example:
choices=[('Issue', 'Issue'), ('Collection', 'Collection')], |
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. |
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
Some functionality is still to do:
Some things still need design / feature development: