Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send email notification when queue reaches a certain size #72

Merged
merged 3 commits into from
Apr 16, 2018

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Apr 13, 2018

fixes #71

Could also add translation keys for the email but think not needed as sent from CLI anyway etc.

Also noticed uninstall was missing but it would currently not uninstall any queue list if a MySQL backend is used... need to create a bug for this. (#73)

@tsteur tsteur added this to the Current sprint milestone Apr 13, 2018
@tsteur tsteur requested a review from diosmosis April 13, 2018 22:58
@tsteur
Copy link
Member Author

tsteur commented Apr 13, 2018

cc @mattab

kept it simple but useful!

@diosmosis
Copy link
Member

Code looks good, build is failing here: https://travis-ci.org/matomo-org/plugin-QueuedTracking/jobs/366338040 due to DEFAULT_NOTIFY_EMAILS value being an array. Will be able to test locally Sunday.

@tsteur
Copy link
Member Author

tsteur commented Apr 14, 2018

Ouch yeah... I'll fix it tomorrow.

@tsteur
Copy link
Member Author

tsteur commented Apr 15, 2018

test fixed

@diosmosis
Copy link
Member

Couple things I noticed testing:

  1. I noticed that the email will contain the sizes for every queue, but will also say that the queue size threshold has been reached. This might suggest to a user that there's a bug if they see that one queue is over the threshold, but the other queues are fine.

To make it clearer, do you think it would be a good idea to differentiate between which queues are ok which aren't. Like having two sections:

This is a notification that the threshold %s for a single queue has been reached.

The following queue sizes are greater than the threshold:
- ...
- ...
- ...

The remaining queue sizes, which are below the threshold, are listed below:
- ...
- ...
  1. I think it might be useful for debugging/testing to add some info/debug logs to notifyQueueSize(). Eg, https://github.com/matomo-org/plugin-QueuedTracking/pull/72/files#diff-e643977fa3ad1b1cee4d42603acb0c44R52 & https://github.com/matomo-org/plugin-QueuedTracking/pull/72/files#diff-e643977fa3ad1b1cee4d42603acb0c44R71. Would also put one https://github.com/matomo-org/plugin-QueuedTracking/pull/72/files#diff-e643977fa3ad1b1cee4d42603acb0c44R44 but not sure how really useful that one would be. Might also be good to have a warning in the UI or somewhere else if the threshold is set to 0, though I suppose that wouldn't happen accidentally.

@tsteur
Copy link
Member Author

tsteur commented Apr 15, 2018

image

done

@tsteur tsteur merged commit 7b467c7 into master Apr 16, 2018
@tsteur tsteur deleted the notifyemail branch April 16, 2018 01:11
@innocraft-automation innocraft-automation removed this from the Current sprint milestone Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send email notification when queue is large
3 participants