URB-3702: Set up cron4plone for notice import - #596
Conversation
📝 WalkthroughWalkthroughThis PR adds a migration upgrade step that registers a cron4plone job for ChangesCron4plone Notice Import Setup
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant GenericSetup
participant update_290
participant ICronConfiguration
GenericSetup->>update_290: run setup_cron4plone_notice_import(context)
update_290->>ICronConfiguration: queryUtility(cron4plone_config)
ICronConfiguration-->>update_290: cron_cfg
update_290->>update_290: check for import-from-notice cron entry
update_290->>ICronConfiguration: append entry and update cron_cfg.cronjobs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Products/urban/migration/update_290.py (2)
648-662: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using the module-level logger instead of creating a new one.
The function creates
logging.getLogger("urban: Setup cron4plone notice import")at line 651, while a module-levellogger = logging.getLogger("urban: migrations")already exists at line 30. Using the module-level logger keeps log filtering consistent with other migration steps.♻️ Proposed refactor
def setup_cron4plone_notice_import(context): - logger = logging.getLogger("urban: Setup cron4plone notice import") - cron_cfg = queryUtility( ICronConfiguration, name="cron4plone_config", context=api.portal.get() )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Products/urban/migration/update_290.py` around lines 648 - 662, The setup_cron4plone_notice_import migration step creates its own logger instead of using the existing module-level logger, which makes migration logging inconsistent. Update this function to reuse the module-level logger already defined in the migration module rather than calling logging.getLogger again, and keep the existing info message behavior unchanged.
659-659: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse unpacking instead of list concatenation.
Ruff flags
list(cron_cfg.cronjobs) + [line_to_add]as RUF005. Prefer[*list(cron_cfg.cronjobs), line_to_add]for idiomatic Python.♻️ Proposed fix
- new_list = list(cron_cfg.cronjobs) + [line_to_add] + new_list = [*list(cron_cfg.cronjobs), line_to_add]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Products/urban/migration/update_290.py` at line 659, The cronjobs list construction in the migration logic should use unpacking instead of list concatenation to satisfy Ruff RUF005. Update the assignment in the code path that builds new_list from cron_cfg.cronjobs and line_to_add so it uses the unpacking form, keeping the same behavior while making the expression idiomatic. Use the existing cron_cfg.cronjobs and new_list symbols to locate the change.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Products/urban/migration/update_290.py`:
- Around line 648-662: The setup_cron4plone_notice_import migration step creates
its own logger instead of using the existing module-level logger, which makes
migration logging inconsistent. Update this function to reuse the module-level
logger already defined in the migration module rather than calling
logging.getLogger again, and keep the existing info message behavior unchanged.
- Line 659: The cronjobs list construction in the migration logic should use
unpacking instead of list concatenation to satisfy Ruff RUF005. Update the
assignment in the code path that builds new_list from cron_cfg.cronjobs and
line_to_add so it uses the unpacking form, keeping the same behavior while
making the expression idiomatic. Use the existing cron_cfg.cronjobs and new_list
symbols to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4c5e572-373a-457c-be37-8c8b9e847c52
📒 Files selected for processing (6)
news/URB-3702.featuresrc/Products/urban/browser/notice_settings.pysrc/Products/urban/migration/update_290.pysrc/Products/urban/migration/upgrades_290.zcmlsrc/Products/urban/profiles/default/metadata.xmlsrc/Products/urban/setuphandlers.py
mpeeters
left a comment
There was a problem hiding this comment.
LGTM, Can you please fix conflicts
Note: metadata version goes from 2916 to 2918 because 2917 was forgotten in another PR.
Summary by CodeRabbit
New Features
Chores