Journals can have a publishing status in addition to being hidden from the press #4988

joemull · 2025-09-29T15:41:41Z

Closes #3970.
Fixes #4701.
Fixes #4418.
Fixes #4987.

For an explanation of decisions see the documentation.

ajrbyers

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.

ajrbyers · 2025-09-30T08:33:43Z

src/api/oai/views.py

 A django implementation of the OAI-PMH interface
 """

+import warnings


Is this imported to fix something else? It doesn't look like its used in the diff.

Just cruft from making a change and then deleting it

ajrbyers · 2025-09-30T08:37:34Z

src/comms/views.py

+    all_tags = models.Tag.objects.all()
+
+    if presswide or request.model_content_type.model == "press":
+        Journal = apps.get_model("journal.Journal")


Is there a circular import if you import journal.models in this module?

Was being overly defensive due to journal models' tendency to create circular imports. Will change this and other instances where I can.

ajrbyers · 2025-09-30T08:38:12Z

src/comms/views.py

        )
        tag = get_object_or_404(models.Tag, text=unquoted_tag)

+    all_tags = (


Why was this moved?

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.

ajrbyers · 2025-09-30T08:42:04Z

src/core/models.py

+            articles = articles.filter(journal=request.journal)
+        else:
+            articles = articles.filter(journal__hide_from_press=False)
+        Journal = apps.get_model("journal.Journal")


Same questions as before, why are we using apps.get_model here?

This is actually best practice to avoid circular imports, I like it and we should use it more often (but only where code is unit tested, as the lack of a top level import statement will no longer be enough to catch a bad import exception)

ajrbyers · 2025-09-30T08:42:34Z

src/identifiers/logic.py


    use_crossref, test_mode, missing_settings = check_crossref_settings(journal)

+    Journal = apps.get_model("journal.Journal")


Same questions as before.

src/themes/clean/templates/press/press_journal_set.html

src/utils/importers/ojs.py

src/utils/importers/shared.py

src/utils/importer.py

src/utils/logic.py

joemull · 2025-09-30T16:13:05Z

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.

ajrbyers · 2025-10-22T08:41:11Z

@mauromsl can you take a look at this? In particular around #4988 (comment) as we need a strategy on this.

mauromsl

Thanks, really appreciate the clear documentation and tests.

Added a couple small comments in line but generally looks good

mauromsl · 2025-11-06T08:32:59Z

src/core/models.py

+            articles = articles.filter(journal=request.journal)
+        else:
+            articles = articles.filter(journal__hide_from_press=False)
+        Journal = apps.get_model("journal.Journal")


This is actually best practice to avoid circular imports, I like it and we should use it more often (but only where code is unit tested, as the lack of a top level import statement will no longer be enough to catch a bad import exception)

mauromsl · 2025-11-06T08:37:46Z

src/journal/models.py

    hide_from_press = models.BooleanField(default=False)

+    class PublishingStatus(models.TextChoices):
+        ACTIVE = "active", _("Active")


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.

mauromsl · 2025-11-06T08:47:02Z

src/press/models.py

+    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,
+            )
+        )


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.

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

def _apply_ordering(self, queryset):

# Check for custom related name (there should be a

# .get_related_name() but I can't find anything like it)

related_name = self.source_field.remote_field.related_name

if not related_name:

related_name = self.through._meta.model_name

# equivalent of my_object.relation.all().order_by(related_name)

return queryset.extra(order_by=[related_name])

mauromsl · 2025-11-06T08:51:00Z

src/themes/clean/templates/press/press_journal_set.html

+            {% if level == "deep" %}
+              <h3 class="journal-name">{{ journal.name }}</h3>
+            {% else %}
+              <h2 class="journal-name">{{ journal.name }}</h2>
+            {% endif %}


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?

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?

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.

joemull added 3 commits September 29, 2025 16:24

feat #3970 Introduce journal publishing status

7e2633e

feat #3970 Fix, test, and document hide_from_press and status

f5c42b9

feat #3970 Edit documentation

7117805

joemull requested review from ajrbyers and mauromsl and removed request for mauromsl September 29, 2025 15:54

joemull assigned ajrbyers Sep 29, 2025

joemull mentioned this pull request Sep 29, 2025

Test journals use test server openlibhums/datacite#20

Open

ajrbyers requested changes Sep 30, 2025

View reviewed changes

ajrbyers assigned joemull and unassigned ajrbyers Sep 30, 2025

joemull added 2 commits September 30, 2025 16:37

chore: fix intermittent test interference between pks

f350126

chore #3970 Clean up imports

9dc1fc9

joemull assigned ajrbyers and unassigned joemull Sep 30, 2025

fix #4987 Update use of random.sample for python 3.11

6bf0c83

joemull requested a review from ajrbyers October 2, 2025 11:35

ajrbyers requested review from mauromsl and removed request for ajrbyers October 22, 2025 08:40

ajrbyers assigned mauromsl and unassigned ajrbyers Oct 22, 2025

mauromsl requested changes Nov 6, 2025

View reviewed changes

mauromsl assigned joemull and unassigned mauromsl Nov 6, 2025

WIP on JournalManager

1978afb

joemull assigned mauromsl and unassigned joemull Nov 11, 2025

joemull requested a review from mauromsl November 11, 2025 13:05


		use_crossref, test_mode, missing_settings = check_crossref_settings(journal)

		Journal = apps.get_model("journal.Journal")

	def _apply_ordering(self, queryset):
	# Check for custom related name (there should be a
	# .get_related_name() but I can't find anything like it)
	related_name = self.source_field.remote_field.related_name
	if not related_name:
	related_name = self.through._meta.model_name

	# equivalent of my_object.relation.all().order_by(related_name)
	return queryset.extra(order_by=[related_name])

Journals can have a publishing status in addition to being hidden from the press #4988

Are you sure you want to change the base?

Journals can have a publishing status in addition to being hidden from the press #4988

Uh oh!

Conversation

joemull commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajrbyers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joemull commented Sep 30, 2025

Uh oh!

ajrbyers commented Oct 22, 2025

Uh oh!

mauromsl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joemull commented Sep 29, 2025 •

edited

Loading