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
ajrbyers
left a comment
ajrbyers
left a comment
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.
Thanks Andy--I've made some changes and added some replies above. |
|
@mauromsl can you take a look at this? In particular around #4988 (comment) as we need a strategy on this. |
mauromsl
left a comment
mauromsl
left a comment
There was a problem hiding this comment.
Thanks, really appreciate the clear documentation and tests.
Added a couple small comments in line but generally looks good
| hide_from_press = models.BooleanField(default=False) | ||
|
|
||
| class PublishingStatus(models.TextChoices): | ||
| ACTIVE = "active", _("Active") |
There was a problem hiding this comment.
Either works. Some constants make sense to exist inside a model because they are "owned" by a model. Others are shared across models and other APIs and makes sense for them to exist in an external namespace, easy to reach by all without leading to circular dependencies (think review decisions)
In case of doubt, an external namespace will be more resilient to future changes.
src/press/models.py
Outdated
| def apply_journal_ordering_az(self, journals): | ||
| """ | ||
| Order a queryset of journals A-Z on English-language journal name. | ||
| Note that this does not support multilingual journal names: | ||
| more work is needed on django-modeltranslation to | ||
| support Django subqueries. | ||
| :param journals: Queryset of Journal objects | ||
| """ | ||
| localized_column = build_localized_fieldname( | ||
| "value", | ||
| settings.LANGUAGE_CODE, # Assumed to be 'en' in default config | ||
| ) | ||
| name = core_models.SettingValue.objects.filter( | ||
| journal=models.OuterRef("pk"), | ||
| setting__name="journal_name", | ||
| ) | ||
| journals.annotate( | ||
| journal_name=models.Subquery( | ||
| name.values_list(localized_column, flat=True)[:1], | ||
| output_field=models.CharField(), | ||
| ) | ||
| ) | ||
| return journals.order_by("journal_name") | ||
|
|
||
| def apply_journal_ordering(self, journals): | ||
| if self.order_journals_az: | ||
| return self.apply_journal_ordering_az(journals) | ||
| else: | ||
| # Journals will already have been ordered according to Meta.ordering | ||
| return journals | ||
|
|
||
| @property | ||
| def journals_az(self): | ||
| """ | ||
| Deprecated. Use `journals` with Press.order_journals_az turned on. | ||
| """ | ||
| warnings.warn( | ||
| "`Press.journals_az` is deprecated. " | ||
| "Use `Press.journals` with Press.order_journals_az turned on." | ||
| ) | ||
| Journal = apps.get_model("journal", "Journal") | ||
| return self.apply_journal_ordering_az(Journal.objects.all()) | ||
|
|
||
| @property | ||
| def public_journals(self): | ||
| Journal = apps.get_model("journal.Journal") | ||
| """ | ||
| Get all journals that are not hidden from the press | ||
| or designated as conferences. | ||
| Do not apply ordering yet, | ||
| since the caller may filter the queryset. | ||
| """ | ||
| Journal = apps.get_model("journal", "Journal") | ||
| return Journal.objects.filter( | ||
| hide_from_press=False, | ||
| is_conference=False, | ||
| ).order_by("sequence") | ||
| ) | ||
|
|
||
| @property | ||
| def public_active_journals(self): | ||
| """ | ||
| Get all journals that are visible to the press | ||
| and marked as 'Active' or 'Test' in the publishing status field. | ||
| Note: Test journals are included so that users can test the journal | ||
| list safely. A separate mechanism exists to hide them from the press | ||
| once the press enters normal operation: | ||
| Journal.hide_from_press. | ||
| """ | ||
| Journal = apps.get_model("journal.Journal") | ||
| return self.apply_journal_ordering( | ||
| self.public_journals.filter( | ||
| status__in=[ | ||
| Journal.PublishingStatus.ACTIVE, | ||
| Journal.PublishingStatus.TEST, | ||
| ] | ||
| ) | ||
| ) | ||
|
|
||
| @property | ||
| def public_archived_journals(self): | ||
| """ | ||
| Get all journals that are visible to the press | ||
| and marked as 'Archived' in the publishing status field. | ||
| """ | ||
| Journal = apps.get_model("journal.Journal") | ||
| return self.apply_journal_ordering( | ||
| self.public_journals.filter( | ||
| status=Journal.PublishingStatus.ARCHIVED, | ||
| ) | ||
| ) | ||
|
|
||
| @property | ||
| def public_coming_soon_journals(self): | ||
| """ | ||
| Get all journals that are visible to the press | ||
| and marked as 'Coming soon' in the publishing status field. | ||
| """ | ||
| Journal = apps.get_model("journal.Journal") | ||
| return self.apply_journal_ordering( | ||
| self.public_journals.filter( | ||
| status=Journal.PublishingStatus.COMING_SOON, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
These sound more like either individual custom managers or a single custom manager with multiple methods. The rule of thumb is to ask oneself, do my model instances need to invoke this property or is this a special way of querying model-instances? If the answer is the latter, it is a good indication that you are writing a Manager method rather than a Model property.
There was a problem hiding this comment.
Yeah, I agree a manager works well. I've gone ahead and switched everything over to one.
However, I am now running into a bug where the order_by method is not recognizing that journal_name has been added as an annotation. This didn't happen when it was a press method, and I don't think I've introduced anything that would have changed the behavior.
I've pushed a WIP--do you know what's going on?
My only guess is it's something with the order of applying ordering in the manager methods, maybe similar to whatever was behind this use of extra() rather than order_by():
janeway/src/core/model_utils.py
Lines 282 to 290 in 1978afb
There was a problem hiding this comment.
@mauromsl would appreciate a second glance at this comment.
| {% if level == "deep" %} | ||
| <h3 class="journal-name">{{ journal.name }}</h3> | ||
| {% else %} | ||
| <h2 class="journal-name">{{ journal.name }}</h2> | ||
| {% endif %} |
There was a problem hiding this comment.
This doesn't feel very intuitive to me and works for this very specific set of templates, but I don't think would be very reusable (I would like to avoid someone adding a level == "very-deep" entry here). Can we use header == "h3"` or similar, if that is the intention?
There was a problem hiding this comment.
I like header = "h3" too, but we've got 20 or so templates that use the "level" + "deep" convention now, so I was being consistent with those. They were introduced by Steph during the breadcrumbs updates. How about you introduce a separate issue to change them?
There was a problem hiding this comment.
I'll second this - its consistent with the the template base as it is but lets open an issue to examine it and potentially change it.
Closes #3970.
Fixes #4701.
Fixes #4418.
Fixes #4987.
For an explanation of decisions see the documentation.