per-resource notifications #15

[cezio]

partial fix #15

This patch adds per-resource notifications. Notifications have separate Recipient model, which is m2m (through ResourceNotification) with Resource model.

Recipient contains two main fields:

• channel - medium type (for now email only)
• location - value that identifies recipient for given channel. value passed to location will be validated when new instance is created.
  Recipient model is not related to any user, so resource's owner can set arbitrary email (or any other recipient) to receive notifications. Recipient doesn't have to be GHC user to get notifications (or can be non-human, like slack bot).

Each time Resource's recipients are modified, unused Recipients are removed.

In UI, recipients are added with input field (for email), with autocomplete, which suggests from values added for resources owned by current user.

I've included test for model api, and support for recipients in data export.

Other changes, improvements to code base:

• included wtforms in requirements
• migrations are regular packages now (added __init__.py)
• replaced import hack for alembic_helpers in migrations with regular import
• included jqueryui/autocomplete (js + css) to static files
• added translations to repo (they are pretty static, no need to generate them for deployment)

[justb4]

Thanks, a valuable addition! Pls give me some time to grasp changes, in particular the intended design. Good idea to split into recipient and resourcenotification and thus allow multiple recipient types (email, Slack etc) and reusable destinations. Shouldn't recipient be explicitly related to user? I.e. a common scenario, e.g. in UptimeRobot, is that a user maintains a list of recipients and selects from that list when adding a notification for a resource. Or is this list implicitly gathered from existing resourcenotifications via resource? What then happens if a resource is deleted?

[cezio]

Yes, flake8 can be annoying, we may want to increase line length (what line-len do you folks use in GeoNode?).

Afair, GeoNode uses strict pep8 line width, but I'd say 120 chars at least is a sane value.

[tomkralidis]

I’d prefer pep8 strict for line length if possible.

[cezio]

Shouldn't recipient be explicitly related to user? I.e. a common scenario, e.g. in UptimeRobot, is that a user maintains a list of recipients and selects from that list when adding a notification for a resource. Or is this list implicitly gathered from existing resourcenotifications via resource? What then happens if a resource is deleted?

I've added some rationale in PR's description. Could you review?

[justb4]

Design and overall approach ok. To be resolved (order of importance):

1. upgrading existing GHC installations: looks like GHC_NOTIFICATIONS_EMAIL is no longer used. Upgraded GHC will have no Recipients for existing Resources hence no notifications (?). Suggestion: initialize Recipients from GHC_NOTIFICATIONS_EMAIL within Alembic upgrade? (only if GHC_NOTIFICATIONS is True).
2. GHC_NOTIFICATIONS_EMAIL also used in Dockerfile (and possibly other places)
3. documentation: update GHC_NOTIFICATIONS_EMAIL texts.
4. docs: also data model update needed
5. suggestion: if possible: add a webhook and slack (Slack has special POST syntax and JSON format IMHO) Enum, also to prove this approach works.

[justb4]

Sorry, did not used Review form, seee comments:
#187 (comment)

[cezio]

upgrading existing GHC installations: looks like GHC_NOTIFICATIONS_EMAIL is no longer used. Upgraded GHC will have no Recipients for existing Resources hence no notifications (?). Suggestion: initialize Recipients from GHC_NOTIFICATIONS_EMAIL within Alembic upgrade? (only if GHC_NOTIFICATIONS is True).

True. Other way would be to keep GHC_NOTIFICATIONS_EMAIL and use it along with per-resource notification emails. I imagine that sometimes it's better to set one global notification target, than having recipients set for each resource. Would that work?

[justb4]

On 26-02-18 19:10, Cezary Statkiewicz wrote: upgrading existing GHC installations: looks like GHC_NOTIFICATIONS_EMAIL is no longer used. Upgraded GHC will have no Recipients for existing Resources hence no notifications (?). Suggestion: initialize Recipients from GHC_NOTIFICATIONS_EMAIL within Alembic upgrade? (only if GHC_NOTIFICATIONS is True). True. Other way would be to keep |GHC_NOTIFICATIONS_EMAIL| and use it along with per-resource notification emails. I imagine that sometimes it's better to set one global notification target, than having recipients set for each resource. Would that work?

That is even better: one could get either/or. Best also to make that clear in doc, maybe UI. Many GHC installs have just one or more emails for all Resources.

[cezio]

upgrading existing GHC installations: looks like GHC_NOTIFICATIONS_EMAIL is no longer used. Upgraded GHC will have no Recipients for existing Resources hence no notifications (?). Suggestion: initialize Recipients from GHC_NOTIFICATIONS_EMAIL within Alembic upgrade? (only if GHC_NOTIFICATIONS is True).
GHC_NOTIFICATIONS_EMAIL also used in Dockerfile (and possibly other places)
documentation: update GHC_NOTIFICATIONS_EMAIL texts.

I've restored use of GHC_NOTIFICATIONS_EMAIL config var, so it's used as global notification emails, as discussed above.

docs: also data model update needed

Done. I've also added source file for diagram for future use.

suggestion: if possible: add a webhook and slack (Slack has special POST syntax and JSON format IMHO) Enum, also to prove this approach works.

I've added simple webhook implementation (model is slightly improved, notifications logic decoupled from detecting change, ui handling for multiple webhooks configuration is added, tests and docs are updated).

Could you review?

[justb4]

Tested by pulling in branch 15_resource_notifications. Also to notify @tomkralidis .

Per Resource email and webhook-notification (using https://requestb.in, very handy) confirmed working, though these issues were found:

• DB migration: using SQLite OK, but error (on column recipient.channel) for PostgreSQL backend:

self.preparer.format_type(type_),
File "/usr/local/lib/python2.7/site-packages/sqlalchemy/dialects/postgresql/base.py", line 1888, in format_type
raise exc.CompileError("PostgreSQL ENUM type requires a name.")
sqlalchemy.exc.CompileError: PostgreSQL ENUM type requires a name.

The Enum type should have a kw-arg name like in models.py L228:

    channel = DB.Column(DB.Enum(*TYPES, name="recp_chan_types"), default=TYPE_EMAIL, nullable=False)

and in migration code in bb91fb332c36.py:

     sa.Column('channel', sa.Enum('email', 'webhook', name="recp_chan_types"), nullable=False),

See also http://docs.sqlalchemy.org/en/latest/core/type_basics.html
name – The name of this type. This is required for PostgreSQL and any future supported database which requires an explicitly named type, or an explicitly named constraint in order to generate the type and/or a table that uses it. If a PEP-435 enumerated class was used, its name (converted to lower case) is used by default.

Upgrade Postgres ok with these fixes.

• see above: 'webhook' should be in Enum definition in bb91fb332c36.py

• edit a Resource. Click 'Save' without filling in email, always error: Save: ('Bad value: Invalid email address.', ValidationError(u'Invalid email address.',)) Looks like at least one email is always required. Especially existing installations may require only the global notification email.

• not clear how to separate email addresses in Resource Edit form (comma-separated), give suggestion text.

• specifying multiple webhooks in Resource Edit form: only first webhook is stored and notified.

[cezio]

Thanks for detailed review!

see above: 'webhook' should be in Enum definition in bb91fb332c36.py

Right. I was checking with sqlite only. I knew that PG supports enums, but didn't know about that subtle difference in SA. Should be fixed.

edit a Resource. Click 'Save' without filling in email, always error: Save: ('Bad value: Invalid email address.', ValidationError(u'Invalid email address.',)) Looks like at least one email is always required. Especially existing installations may require only the global notification email.

Should be fixed.

not clear how to separate email addresses in Resource Edit form (comma-separated), give suggestion text.

I've added label to template, updated docs and translations.

specifying multiple webhooks in Resource Edit form: only first webhook is stored and notified.

I couldn't reproduce this. Did you add subsequent webhooks with +? I've added green/red colors to those widgets, as they may be hard to notice.

[justb4]

Tested new commits: 23143cb..5feafb5 on march 5, 2018, 15:00 UTC comments below. On 05-03-18 14:11, Cezary Statkiewicz wrote: see above: 'webhook' should be in Enum definition in bb91fb332c36.py Right. I was checking with sqlite only. I knew that PG supports enums, but didn't know about that subtle difference in SA. Should be fixed. Ok now. edit a Resource. Click 'Save' without filling in email, always error: Save: ('Bad value: Invalid email address.', ValidationError(u'Invalid email address.',)) Looks like at least one email is always required. Especially existing installations may require only the global notification email. Should be fixed. Ok now. not clear how to separate email addresses in Resource Edit form (comma-separated), give suggestion text. I've added label to template, updated docs and translations. Ok, for label only in my case the label "You can enter multiple emails separated with comma" is above (Resource) "Name" Input Field, not intended "Notify emails" Field. specifying multiple webhooks in Resource Edit form: only first webhook is stored and notified. I couldn't reproduce this. Did you add subsequent webhooks with |+|? I've added green/red colors to those widgets, as they may be hard to notice. Is ok. Yes, forgot entering |+|).

[cezio]

Ok, for
label only in my case the label "You can enter multiple emails separated
with comma" is above (Resource) "Name" Input Field, not intended "Notify
emails" Field.

fixed

[justb4]

On 05-03-18 19:23, Cezary Statkiewicz wrote: Ok, for label only in my case the label "You can enter multiple emails separated with comma" is above (Resource) "Name" Input Field, not intended "Notify emails" Field. fixed

Ok, confirmed! Wait for merge!

[simboss]

@justb4 It is not clear for me if you are still requesting chamges on this PR. Can you , please, clarify?

[justb4]

@cezio @simboss we are clear to merge. Thanks for patience. @tomkralidis and I needed to make sure we can perform the DB upgrade on the demo server: http://demo.geohealthcheck.org which is hooked to commits on master. Today lacked time because of WFS 3 hackathon and regular work. Merge upcoming..

[justb4]

PR running now on http://demo.geohealthcheck.org . Had some trouble due to outdated pip. New forms, upgraded DB and docs e.g. http://docs.geohealthcheck.org/en/latest/architecture.html are all there. Notifications in general are disabled in demo.

[simboss]

@justb4 no worries, I just wanted to make sure you weren't expecting any actions from us.