Cron export by pilz · Pull Request #216 · syslabcom/recensio.plone

pilz · 2026-04-06T11:12:56Z

See https://github.com/syslabcom/scrum/issues/3960

Migrated code to send XML export over SFTP Refs. syslabcom/scrum#3960

ale-rt · 2026-04-08T06:28:50Z

Can you at least start by fixing the pre-commit?

Copilot

Pull request overview

Adds a bulk “cron-style” export subsystem to generate XML/CSV export artifacts, provide browser endpoints to run/export them, and support DOI (da|ra) registration flows (per #3960).

Changes:

Introduces exporter implementations (Chronicon/BVID/MissingBVID/LZA) plus DOI registration helpers and status objects.
Adds browser views + ZCML registrations for XML representations and export endpoints (metadata-export, chronicon-export, DOI update, reset flags).
Adds extensive integration tests and export documentation, plus a new SFTP dependency.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
`src/recensio/plone/export.py`	New exporter implementations, ZIP/CSV generation, DOI registration helper functions
`src/recensio/plone/browser/export.py`	New browser views to run bulk exports, upload Chronicon ZIP via SFTP, trigger DOI update, reset LZA flags
`src/recensio/plone/browser/xml.py`	Adds XML representation views for publication/volume/issue/reviews
`src/recensio/plone/browser/configure.zcml`	Registers new XML views and export-related browser endpoints
`src/recensio/plone/configure.zcml`	Registers exporter factory utility (currently Chronicon only)
`src/recensio/plone/interfaces.py`	Adds `IRecensioExporter` interface
`src/recensio/plone/config.py`	Adds `REVIEW_TYPES_TO_EXPORT` selection for bulk export traversal
`src/recensio/plone/browser/templates/export_*.pt`	Adds/updates XML templates for export formats (including da
`src/recensio/plone/tests/test_exporters.py`	Adds integration/unit tests for exporters and DOI registration logic
`setup.py`	Adds `paramiko-ng` dependency for SFTP uploads
`docs/recensio-exports.md`	Adds an overview document for the export subsystem
`CHANGES.rst`	Changelog entry for export revival work

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pilz · 2026-04-13T13:18:09Z

Can you at least start by fixing the pre-commit?

Please help me understand what I did wrong.

I do have my pre-commit installed for recensio, I know because it is talking to me all the time. But I double checked, it is there and linked.

Now why didn't it warn me on the corrections you found?
I made a trivial change to one of the files and tried to commit that. pre-commit stopped me.

So how did the file get into this PR in the first place?

Ah, it was added by Cillian/Ciro who started this in one of the first flow sessions, thinking this was a quick cronjob task. They very likely didn't have the pre-commit installed as they were not set up for recensio nor for developing this.

Why didn't the test pick that up then? The PR check is green?

What is the correct course of action? Should the developer
a) in addition to the hook, also ensure that all files stay checked?
b) ensure that the automated tests also run the hooks --all-files check?
c) something else?

How did you find this in the first place? Did you run the pre-commit hook on all files manually out of routine?

ale-rt · 2026-04-13T13:24:49Z

Can you at least start by fixing the pre-commit?

Please help me understand what I did wrong.

I do have my pre-commit installed for recensio, I know because it is talking to me all the time. But I double checked, it is there and linked.

Now why didn't it warn me on the corrections you found? I made a trivial change to one of the files and tried to commit that. pre-commit stopped me.

So how did the file get into this PR in the first place?

Ah, it was added by Cillian/Ciro who started this in one of the first flow sessions, thinking this was a quick cronjob task. They very likely didn't have the pre-commit installed as they were not set up for recensio nor for developing this.

Why didn't the test pick that up then? The PR check is green?

What is the correct course of action? Should the developer a) in addition to the hook, also ensure that all files stay checked? b) ensure that the automated tests also run the hooks --all-files check? c) something else?

How did you find this in the first place? Did you run the pre-commit hook on all files manually out of routine?

It seems to me that many questions you had are rhetorical and you find the solution yourself.
I will start from the bottom.

The CI checks the pre-commit hook:
https://github.com/syslabcom/recensio.plone/actions/runs/24345533981/job/71085240985?pr=216

Let me know if you want some more info

pilz · 2026-04-13T18:16:58Z

It seems to me that many questions you had are rhetorical and you find the solution yourself.

The idea was to allow you to see where I am coming from and what I tried.

I will start from the bottom.

The CI checks the pre-commit hook: https://github.com/syslabcom/recensio.plone/actions/runs/24345533981/job/71085240985?pr=216

Let me know if you want some more info

Yes. Your phrasing "Can you at least start by fixing the pre-commit?" suggests with using "at least" that I have disregarded basic quality principles, while I thought I performed quite well and also the test was green.

What should I have done to detect this pre-commit issue on files I didn't touch?

ale-rt · 2026-04-14T06:51:48Z

What should I have done to detect this pre-commit issue on files I didn't touch?

Maybe check the files you did not touch before starting working on that?

ale-rt · 2026-04-14T07:01:11Z

+      name="metadata-export"
+      for="Products.CMFPlone.interfaces.IPloneSiteRoot"
+      class=".export.MetadataExport"
+      permission="zope2.View"


Copilot is very right

… 6 migration

Copilot

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T17:44:07Z

+                        <input class="form-check-input"
+                               id="query-date-yesterday"
+                               checked="${python:'checked' if checked==yesterday else ''}"
+                               name="created.query:record:date"
+                               type="radio"
+                               value="${yesterday}"
+                        />


Same boolean-attribute issue for the date radios: checked="" is still treated as checked. Use a TAL attribute that omits checked when the condition is false (set to None/nothing).

Copilot · 2026-04-16T17:44:08Z

+      permission="cmf.ModifyPortalContent"
+      />
+  <browser:page
+      name="chronicon-export"
+      for="Products.CMFPlone.interfaces.IPloneSiteRoot"
+      class=".export.ChroniconExport"
+      permission="cmf.ModifyPortalContent"
+      />
+  <browser:page
+      name="dara_update"
+      for="recensio.plone.interfaces.IReview"
+      class=".export.DaraUpdate"
+      permission="cmf.ModifyPortalContent"
+      />
+  <browser:page
+      name="reset-lza-export-flag"
+      for="*"
+      class=".export.ResetLZAExportFlag"
+      permission="cmf.ModifyPortalContent"


@@metadata-export disables CSRF protection and performs site-wide writes, but it’s protected only by cmf.ModifyPortalContent, which is often granted to non-admin editorial roles. Consider restricting this (and the other export helpers) to a stronger admin permission (e.g. cmf.ManagePortal / a dedicated permission) and/or requiring POST for side-effecting operations.

Suggested change

permission="cmf.ModifyPortalContent"

/>

<browser:page

name="chronicon-export"

for="Products.CMFPlone.interfaces.IPloneSiteRoot"

class=".export.ChroniconExport"

permission="cmf.ModifyPortalContent"

/>

<browser:page

name="dara_update"

for="recensio.plone.interfaces.IReview"

class=".export.DaraUpdate"

permission="cmf.ModifyPortalContent"

/>

<browser:page

name="reset-lza-export-flag"

for="*"

class=".export.ResetLZAExportFlag"

permission="cmf.ModifyPortalContent"

permission="cmf.ManagePortal"

/>

<browser:page

name="chronicon-export"

for="Products.CMFPlone.interfaces.IPloneSiteRoot"

class=".export.ChroniconExport"

permission="cmf.ManagePortal"

/>

<browser:page

name="dara_update"

for="recensio.plone.interfaces.IReview"

class=".export.DaraUpdate"

permission="cmf.ManagePortal"

/>

<browser:page

name="reset-lza-export-flag"

for="*"

class=".export.ResetLZAExportFlag"

permission="cmf.ManagePortal"

Copilot · 2026-04-16T17:44:08Z

+      template="templates/export_rj.pt"
+      permission="zope.Public"
+      />
+


REVIEW_TYPES_TO_EXPORT includes “Review Article Collection”, and ChroniconExporter.add_review() calls review.restrictedTraverse('@@xml')(). However, there is no @@xml browser:page registered here for IReviewArticleCollection (nor IReviewExhibition). Those reviews will raise NotFound during export and be skipped. Register matching @@xml views/templates for the missing review types, or remove them from the export set.

Suggested change

<browser:page

name="xml"

for="recensio.plone.content.review_article_collection.IReviewArticleCollection"

class=".xml.XMLRepresentation"

template="templates/export_rj.pt"

permission="zope.Public"

/>

<browser:page

name="xml"

for="recensio.plone.content.review_exhibition.IReviewExhibition"

class=".xml.XMLRepresentation"

template="templates/export_rj.pt"

permission="zope.Public"

/>

Copilot · 2026-04-16T17:44:09Z

+BVIDExporterFactory = Factory(BVIDExporter, IFactory, "exporter")
+MissingBVIDExporterFactory = Factory(MissingBVIDExporter, IFactory, "exporter")
+ChroniconExporterFactory = Factory(ChroniconExporter, IFactory, "exporter")
+LZAExporterFactory = Factory(LZAExporter, IFactory, "exporter")


The Factory(...) registrations pass IFactory and a string positional argument, which means factory.getInterfaces() will not include IRecensioExporter. As a result, getFactoriesFor(IRecensioExporter) in MetadataExport will return no exporters and the bulk export runner will always report “Nothing to do”. Define these factories without overriding the interfaces (e.g. Factory(BVIDExporter) / Factory(ChroniconExporter)), or explicitly declare IRecensioExporter as the provided interface, so getFactoriesFor(IRecensioExporter) can discover them.

Copilot · 2026-04-16T17:44:09Z

+                        <input class="form-check-input"
+                               id="pt_toggle"
+                               checked="${python:'checked' if all_checked else ''}"
                               name="pt_toggle"
+                               type="checkbox"
                               value="#"
-                               id="pt_toggle"
-                               class="form-check-input"
-                               checked="${python:'checked' if all_checked else ''}" />
-                        <label for="pt_toggle" class="form-check-label"
-                               i18n:translate="label_toggle">Select All/None</label>
+                        />


checked is interpolated with an empty string when false (checked=""), but for HTML boolean attributes the presence of checked still means true. This checkbox will render as checked even when all_checked is false. Use a TAL attribute that omits checked when false (e.g. set it to None/nothing).

Copilot · 2026-04-16T17:44:09Z

+      component=".export.ChroniconExporterFactory"
+      />
+
  <utility


Only chronicon_exporter is registered as an IFactory utility here. MetadataExport uses getFactoriesFor(IRecensioExporter) to discover exporters, so the exporter factories (Chronicon/BVID/MissingBVID/LZA) need to be registered as IFactory utilities and advertise IRecensioExporter in getInterfaces(). Otherwise the bulk export runner won’t find anything to execute.

Suggested change

<utility

<utility

provides="zope.component.interfaces.IFactory"

name="bvid_exporter"

component=".export.BVIDExporterFactory"

/>

<utility

provides="zope.component.interfaces.IFactory"

name="missing_bvid_exporter"

component=".export.MissingBVIDExporterFactory"

/>

<utility

provides="zope.component.interfaces.IFactory"

name="lza_exporter"

component=".export.LZAExporterFactory"

/>

<utility

Copilot · 2026-04-16T17:44:09Z

+                        <input class="form-check-input"
+                               id="portal_type_${repeat/type/number}"
+                               checked="${python:'checked' if ((type in request.get('portal_type', [])) or first_call) else ''}"
+                               name="portal_type:list"
+                               type="checkbox"
+                               value="${type}"
+                        />


Same boolean-attribute issue here: when the expression is false this becomes checked="", which still counts as checked. Use a TAL attribute that omits checked entirely when the condition is false (use None/nothing).

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ess.pt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

ale-rt · 2026-04-17T08:44:55Z

@copilot merge master and switch the changelog entry to use towncrier

Co-authored-by: ale-rt <1300763+ale-rt@users.noreply.github.com>

Copilot · 2026-04-17T08:49:57Z

@copilot merge master and switch the changelog entry to use towncrier

Done in ac84a15: merged main (which introduced the towncrier setup from #233), removed the CHANGES.rst entry, and created news/3960.feature.md with the towncrier fragment for this issue.

cirosilvano and others added 2 commits September 26, 2025 18:18

chronicon-export: migrated code from recensio.policy

2e363d7

Migrated code to send XML export over SFTP Refs. syslabcom/scrum#3960

Merge branch 'main' into cron-export

832f239

pilz requested review from ale-rt, cillianderoiste and cirosilvano April 6, 2026 11:50

ale-rt requested a review from Copilot April 8, 2026 06:38

Copilot started reviewing on behalf of ale-rt April 8, 2026 06:39 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

ale-rt reviewed Apr 14, 2026

View reviewed changes

Comment thread src/recensio/plone/browser/export.py Outdated

Completed the takeover of old code that was left out during the Plone…

4efcc39

… 6 migration

pilz force-pushed the cron-export branch from 9fd8594 to 4efcc39 Compare April 16, 2026 17:29

pilz requested a review from Copilot April 16, 2026 17:36

Copilot started reviewing on behalf of pilz April 16, 2026 17:36 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

pilz and others added 4 commits April 17, 2026 10:39

Update docs/recensio-exports.md

98600f7

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update docs/recensio-exports.md

41aba44

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/recensio/plone/browser/templates/export_container_contextl…

5159365

…ess.pt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge branch 'main' into cron-export

3419605

Copilot started work on behalf of ale-rt April 17, 2026 08:45 View session

Merge main, switch to towncrier, fix export view permissions

ac84a15

Co-authored-by: ale-rt <1300763+ale-rt@users.noreply.github.com>

Copilot finished work on behalf of ale-rt April 17, 2026 08:50

Copilot AI requested a review from ale-rt April 17, 2026 08:50

-  <utility
+  <utility
+      provides="zope.component.interfaces.IFactory"
+      name="bvid_exporter"
+      component=".export.BVIDExporterFactory"
+      />
+  <utility
+      provides="zope.component.interfaces.IFactory"
+      name="missing_bvid_exporter"
+      component=".export.MissingBVIDExporterFactory"
+      />
+  <utility
+      provides="zope.component.interfaces.IFactory"
+      name="lza_exporter"
+      component=".export.LZAExporterFactory"
+      />
+  <utility

Conversation

pilz commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ale-rt commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pilz commented Apr 13, 2026

Uh oh!

ale-rt commented Apr 13, 2026 • edited by pilz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pilz commented Apr 13, 2026

Uh oh!

ale-rt commented Apr 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ale-rt Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ale-rt commented Apr 17, 2026

Uh oh!

Copilot AI commented Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pilz commented Apr 6, 2026 •

edited

Loading

ale-rt commented Apr 13, 2026 •

edited by pilz

Loading