Journals can have a publishing status in addition to being hidden from the press #4988
Conversation
joemull
requested review from
ajrbyers and
mauromsl
and removed request for
mauromsl
There was a problem hiding this comment.
Generally this is great work. I've only done a code review here as I'd like @mauromsl to look over it and then have it come back to me.
There is one major issue I see with this PR. Could you share the reason you’re using apps.get_model throughout this PR? I can see one or two places where it makes sense and in others where it seems a direct import would be better and more consistent.
I've made some comments and suggestions throughout, if you have any queries let me know.
src/api/oai/views.py
Outdated
| A django implementation of the OAI-PMH interface | ||
| """ | ||
|
|
||
| import warnings |
There was a problem hiding this comment.
Is this imported to fix something else? It doesn't look like its used in the diff.
There was a problem hiding this comment.
Just cruft from making a change and then deleting it
src/comms/views.py
Outdated
| all_tags = models.Tag.objects.all() | ||
|
|
||
| if presswide or request.model_content_type.model == "press": | ||
| Journal = apps.get_model("journal.Journal") |
There was a problem hiding this comment.
Is there a circular import if you import journal.models in this module?
There was a problem hiding this comment.
Was being overly defensive due to journal models' tendency to create circular imports. Will change this and other instances where I can.
| ) | ||
| tag = get_object_or_404(models.Tag, text=unquoted_tag) | ||
|
|
||
| all_tags = ( |
There was a problem hiding this comment.
So that it could be filtered by the journal. I realized that it wasn't being filtered, so it could potentially include junk tags from journals that are hidden from the press.
| articles = articles.filter(journal=request.journal) | ||
| else: | ||
| articles = articles.filter(journal__hide_from_press=False) | ||
| Journal = apps.get_model("journal.Journal") |
There was a problem hiding this comment.
Same questions as before, why are we using apps.get_model here?
src/identifiers/logic.py
Outdated
|
|
||
| use_crossref, test_mode, missing_settings = check_crossref_settings(journal) | ||
|
|
||
| Journal = apps.get_model("journal.Journal") |
|
|
||
| <div class="col-lg-8 col-md-10 journal-container"> | ||
| {% for journal in journals %} | ||
| {% setting_var journal 'disable_journal_submission' as submission_disabled %} |
There was a problem hiding this comment.
This is already in the context {{ journal_settings.general.disable_journal_submission }}. No need to fetch it again.
There was a problem hiding this comment.
Thanks--I was just moving this into a new file, but I will make the change.
There was a problem hiding this comment.
This doesn't seem to have anything to do with this PR.
There was a problem hiding this comment.
I was going through the utils/importers files with a fine-toothed comb to look for instances of journal-related querysets, to see if I needed to make changes related to hide_from_press or the test publishing status. As I was doing so, I remembered that we agreed these were deprecated during the ROR work. I'm not misremembering, am I? So that I didn't have to re-check them in a future piece of work on something else, I marked them deprecated.
There was a problem hiding this comment.
This doesn't seem to have anything to do with this PR.
There was a problem hiding this comment.
This doesn't seem to have anything to do with this PR.
src/utils/logic.py
Outdated
| import hmac | ||
| from urllib.parse import SplitResult, urlencode, urlparse, unquote | ||
|
|
||
| from django.apps import apps |
There was a problem hiding this comment.
This is imported and not used.
Thanks Andy--I've made some changes and added some replies above. |
2 participants
Closes #3970.
Fixes #4701.
Fixes #4418.
Fixes #4987.
For an explanation of decisions see the documentation.