Sends email notifications for directory entries with publication date…

[Tschuppi81]

Directory: Delay sending update notifications to subscribers if publication starts in future

TYPE: Feature
LINK: ogc-1825

Checklist

• I have performed a self-review of my code
• I considered adding a reviewer
• I have tested my code thoroughly by hand
  • I have tested sending mails
• I have added tests for my changes/features
  • I am using freezegun for tests depending on date and times

[linear]

OGC-1825 Benachrichtigungen verzögert senden, wenn Publikations-Start in Zukunft

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.72%. Comparing base (10c9a81) to head (c50d9c5).
Report is 1 commits behind head on master.

Additional details and impacted files

Files with missing lines	Coverage Δ
src/onegov/org/cronjobs.py	92.92% <100.00%> (+0.08%)	⬆️
src/onegov/org/models/organisation.py	97.98% <100.00%> (+0.01%)	⬆️
src/onegov/org/views/directory.py	79.91% <100.00%> (+0.09%)	⬆️

... and 3 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10c9a81...c50d9c5. Read the comment docs.

[Tschuppi81]

I got confused as ExtendedDirectoryEntry.published == True did not pass the tests. Any idea why?

[Daverball]

Good question, not sure.

[Daverball]

I think this is currently a bit too complicated, I would use the exact same logic as in reindex_published_models and just send notifications for everything that was published in the last hour, rather than rely on additional metadata.

In fact you can just add another isinstance(obj, ExtendedDirectoryEntry) check to the existing function and trigger the notification in that same loop in order to avoid a redundant query.

Since cronjobs always run at specific intervals there's no chance of triggering too many notifications (there is however a small chance of skipping one during an upgrade)

While storing whether or not the notification was sent seems more robust at first, it also introduces potential confusion for entries that have their publication dates set a second time for republishing and then not triggering a second notification.

So if we wanted to handle that case properly we would need to introduce additional logic elsewhere. It doesn't seem worth the hassle right now.

[Daverball]

Small caveat to my suggestion: Since the existing loop also includes recently unpublished objects you will need to check obj.published in addition to isinstance(obj, ExtendedDirectory).

[Tschuppi81]

Since cronjobs always run at specific intervals there's no chance of triggering too many notifications (there is however a small chance of skipping one during an upgrade)
Yes. Also we could miss sending out notifications in cases where an instance is down for > 1h

I will work on the approach with no metadata

[Daverball]

Yes. Also we could miss sending out notifications in cases where an instance is down for > 1h

If we wanted to address this I think my preference would be to store on Organisation when this cronjob was last run. That's a lot less book-keeping for similar robustness.

So here:

onegov-cloud/src/onegov/org/cronjobs.py

Lines 113 to 114 in f67e97d

	now = utcnow()
	then = now - timedelta(hours=1)

Instead of then being static you would change it to

    now = utcnow()
    then = request.app.org.meta.get('hourly_maintenance_tasks_last_run', now - timedelta(hours=1))

and at the end of the function you do

    request.app.org.meta['hourly_maintenance_tasks_last_run'] = now

This also avoids potentially dropping anything in the small potential second gaps between cronjob runs due to jitter.

[Tschuppi81]

I got it integrated