harmen/hypha

Fork 0

Implemented article status update message #183

Merged

giplt merged 4 commits from user_feedback into master

2019-01-14 20:19:50 +00:00

giplt commented

2019-01-13 01:02:48 +00:00

(Migrated from github.com)

No description provided.

matthijskooijman (Migrated from github.com) requested changes

2019-01-13 14:52:29 +00:00

matthijskooijman (Migrated from github.com) left a comment

This looks very good, it much improves the feedback and the code is a lot cleaner than the old code that hardcoded the links to be added to the messages.

I added one nitpick comment, which is not really essential to fix.

Also, the commit message "Implemented article status update message" seems off - the notification message was already sent, but this commit only improves its content.

Finally, this PR is based on an older master version, so it should be updated (rebased) on top of the current master (e.g. git fetch and then git rebase origin/master). This will likely produce a conflict that must be resolved, due to changes in the language files.

This looks *very* good, it much improves the feedback and the code is a lot cleaner than the old code that hardcoded the links to be added to the messages. I added one nitpick comment, which is not really essential to fix. Also, the commit message "Implemented article status update message" seems off - the notification message was already sent, but this commit only improves its content. Finally, this PR is based on an older master version, so it should be updated (rebased) on top of the current master (e.g. `git fetch` and then `git rebase origin/master`). This will likely produce a conflict that must be resolved, due to changes in the language files.

system/languages/en.php

This string has a trailing space.

matthijskooijman commented

2019-01-17 08:19:35 +00:00

(Migrated from github.com)

I made a stab at a fix, here (just a branch, no PR yet): https://github.com/PlanBCode/hypha/commits/comment-mail-wip

However, reading into the code more, it actually seemed a bit weird that this notification was sent at all. Currently, adding a new comment (review or visitor) both triggers an EVENT_PUBLIC_COMMENT event, which calls onPublicComment() which either sends a confirmation mail (when posted by a visitor), or calls sendNewCommentMessage() (when posted by a user).

I wonder if all this is how it was intended? EVENT_PUBLIC_COMMENT suggests to me "visitor comment", but apparently it is called for review comments too. This does not actually seem necessary (since review comments are always posted by users and never need confirmation), but that might change in the future? In any case, the current code (with this PR merged) will send out a (currently broken, as above) notification for every review comment, and a second notification mail for new blocking discussions (so you get two mails for a single comment in that case).

@giplt, did you intend to send notifications for all review comments? Or did you want to keep the old behaviour of sending a notification for new blocking discussions only and add notifications for all visitor comments only?

I did a bit more testing of this and noted that when you post a review comment, a notification is sent without a commenter name (e.g. "Test: New comment by to 'Testarticle2'"). Turns out that the notification code assumed that a comment has a 'name' attribute, but review comments only have a 'user' attribute with a userid that must be looked up. I made a stab at a fix, here (just a branch, no PR yet): https://github.com/PlanBCode/hypha/commits/comment-mail-wip However, reading into the code more, it actually seemed a bit weird that this notification was sent at all. Currently, adding a new comment (review or visitor) both triggers an `EVENT_PUBLIC_COMMENT` event, which calls `onPublicComment()` which either sends a confirmation mail (when posted by a visitor), or calls `sendNewCommentMessage()` (when posted by a user). I wonder if all this is how it was intended? `EVENT_PUBLIC_COMMENT` suggests to me "visitor comment", but apparently it is called for review comments too. This does not actually seem necessary (since review comments are always posted by users and never need confirmation), but that might change in the future? In any case, the current code (with this PR merged) will send out a (currently broken, as above) notification for *every* review comment, and a second notification mail for new blocking discussions (so you get two mails for a single comment in that case). @giplt, did you intend to send notifications for all review comments? Or did you want to keep the old behaviour of sending a notification for new blocking discussions only and add notifications for all visitor comments only?

matthijskooijman commented

2019-01-17 20:13:13 +00:00

(Migrated from github.com)

The issue noted above is fixed by #192.

I noticed another issue, see https://github.com/PlanBCode/hypha/issues/99#issuecomment-455312177

The issue noted above is fixed by #192. I noticed another issue, see https://github.com/PlanBCode/hypha/issues/99#issuecomment-455312177

dianawi commented

2019-01-19 10:39:46 +00:00

(Migrated from github.com)

I just left a comment on test but do not get an email.

giplt commented

2019-01-19 17:57:19 +00:00

(Migrated from github.com)

Apparently I didn't fully grasp the stream of events that is implemented in the article module, which seems more convoluted than I imagined. Actually I'm not so sure now where and how to implement this. Feels like the more fundamental discussion on a preferred programming paradigm needs to take place first. How do we want the datatype modules to be organised: structured, data/event-driven, other, mixed?

No reviewers

matthijskooijman

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha!183

No description provided.

Rows
Columns