1215664 - [PulseGuardian] Add email addresses for notifications to a Pulse user

Status: RESOLVED FIXED

(Webtools :: Pulse, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Bug 1215464 caused bustage to our Pulse queue for handling Firefox builds and the Firefox UI tests. I was notified about an overgrowing queue yesterday while I was on vacation. Sadly no-one from my team was aware of it. So no-one actually reacted on the issue.

What we want is a queue which can be shared between users, so that we all are getting those messages. As of now this doesn't seem to be possible with PulseGuardian. Maybe Marc can comment on that.

I would suggest that we use our internal mailing list [e-mail] to login to PulseGuardian and to setup the necessary queues. Then everyone subscribed to this list will be notified and escalation can happen.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just to add there is no-one at the moment who is using a list for signin. So I would like to get explicit feedback from Marc if that is what we want to allow.

Comment 2

[Mark Côté [:mcote]]

Note that you need a Persona account to log in.  Not sure if you can sign lists up for Persona, but maybe you can.

I think the better solution is to allow people to add email addresses to notifications for particular Pulse users.  Another option is shared accounts, which we've been working on in bug 1068447.  Unfortunately I have no ETA for those, unless you're interested in helping out with PulseGuardian. :)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Mark Côté [:mcote] from comment #2)
> I think the better solution is to allow people to add email addresses to
> notifications for particular Pulse users. 

It sounds like this would be available but I doubt so, right? Mind shedding a bit more details here? Thanks.

Comment 4

[Mark Côté [:mcote]]

Opening this bug up since there's nothing particularly confidential, and moving to the Pulse component.

(In reply to Henrik Skupin (:whimboo) from comment #3)
> (In reply to Mark Côté [:mcote] from comment #2)
> > I think the better solution is to allow people to add email addresses to
> > notifications for particular Pulse users. 
> 
> It sounds like this would be available but I doubt so, right? Mind shedding
> a bit more details here? Thanks.

No, it's not implemented right now.  I think this could be a good mentored bug, however, so I'll add a user story with details.

Comment 5

[Mike Yao]

Hi all,

I'm new contributor, this is my second bug on this project.

Hi Mark,

Please help to ensure my understanding is correct.

Table: user_emails
Fields: id, user_id(foreign key), email
Question: Move email to own table means it will have multiple emails per user? 

Table: user_queues
Fields: id, queue_id(foreign key), email_id(foreign key)
relationship: many queues to many users 

Thank you.

Comment 6

[Mark Côté [:mcote]]

That's close.

For the first table, I was thinking more something like this:

Table: emails
Fields: id, email

For the second table (user_queues) I would probably call it queue_notifications or queue_extra_emails or something like that.

And then changing the users table to have email_id(foreign key) instead of email(string).  That way a User object still only has one email associated with it (the email they use to log in, as it is now, but stored in a different table), but a queue can have multiple emails (these are extra notification addresses, not representing owners at all).  You'd need to migrate the existing data over as well (probably create an extra column and the new tables, then copy the data over, then remove the old column).

Does that make sense?

Comment 7

[Mike Yao]

(In reply to Mark Côté [:mcote] from comment #6)
> That's close.
> 
> For the first table, I was thinking more something like this:
> 
> Table: emails
> Fields: id, email
> 
> For the second table (user_queues) I would probably call it
> queue_notifications or queue_extra_emails or something like that.
> 
> And then changing the users table to have email_id(foreign key) instead of
> email(string).  That way a User object still only has one email associated
> with it (the email they use to log in, as it is now, but stored in a
> different table), but a queue can have multiple emails (these are extra
> notification addresses, not representing owners at all).  You'd need to
> migrate the existing data over as well (probably create an extra column and
> the new tables, then copy the data over, then remove the old column).
> 
> Does that make sense?

The extra email addresses must be belong to user in Pulsegardian or any addresses?

Comment 8

[Mark Côté [:mcote]]

Any address.  It might be associated with a user, or it might not.  If it is, the single email row should be shared between the queue_notifications table and the user table.  The way I picture this being used is like this:

1. User with email address [e-mail] logs into PulseGuardian via Persona for the first time.  Assuming no one has ever used that address for other notifications, a new entry in the emails table is created and a new entry in the users table linking to the new email table row.
2. User creates a new Pulse user.  This results in a new entry in the pulseusers table associated with the user table row (this is unchanged from how it works now).
3. Using a new UI element (which has to be designed as part of this bug), the user adds a new notification email address to the queue, say, [e-mail].  The intention here is that this might be, say, a mailing list, instead of a personal email, so multiple people can be alerted when the queue gets too big.  This results in a new row in the emails table and a new row in the queue_notifications table tying the new email table row and the queue table row together.
4. At some point the queue grows too large.  An email is sent to both [e-mail] and [e-mail].
5. Some time later, someone logs into PulseGuardian with the address [e-mail] and creates a new account.  The same actions for step 1 happen, except that we don't need to create a new entry in the emails table because [e-mail] already exists in the table.  Instead we just use that row's ID as the email foreign key in the new user table row.

Finally, if nothing is referencing an email address any longer (say, the queue is deleted and no user uses that email address either), the email should be deleted, just to prevent the table from getting larger forever.

Comment 9

[Mike Yao]

(In reply to Mark Côté [:mcote] from comment #8)
> Any address.  It might be associated with a user, or it might not.  If it
> is, the single email row should be shared between the queue_notifications
> table and the user table.  The way I picture this being used is like this:
> 
> 1. User with email address [e-mail] logs into PulseGuardian via
> Persona for the first time.  Assuming no one has ever used that address for
> other notifications, a new entry in the emails table is created and a new
> entry in the users table linking to the new email table row.
> 2. User creates a new Pulse user.  This results in a new entry in the
> pulseusers table associated with the user table row (this is unchanged from
> how it works now).
> 3. Using a new UI element (which has to be designed as part of this bug),
> the user adds a new notification email address to the queue, say,
> [e-mail].  The intention here is that this might be, say, a
> mailing list, instead of a personal email, so multiple people can be alerted
> when the queue gets too big.  This results in a new row in the emails table
> and a new row in the queue_notifications table tying the new email table row
> and the queue table row together.
> 4. At some point the queue grows too large.  An email is sent to both
> [e-mail] and [e-mail].
> 5. Some time later, someone logs into PulseGuardian with the address
> [e-mail] and creates a new account.  The same actions for
> step 1 happen, except that we don't need to create a new entry in the emails
> table because [e-mail] already exists in the table.  Instead
> we just use that row's ID as the email foreign key in the new user table row.
> 
> Finally, if nothing is referencing an email address any longer (say, the
> queue is deleted and no user uses that email address either), the email
> should be deleted, just to prevent the table from getting larger forever.

That makes sense. Thanks Mark.

Comment 10

[Mike Yao]

As suggestion for splitting multiple commits, this is the first commit for migration. 

https://github.com/yaoweizhen/pulseguardian/commit/c13b63c0b2f226b577a99b29699963a22d72de1d

Comment 11

[Mike Yao]

run "alembic upgrade head", if it doesn't work, try to set current migration version correctly - "alembic stamp 641f"

(In reply to Mike Yao from comment #10)
> As suggestion for splitting multiple commits, this is the first commit for
> migration. 
> 
> https://github.com/yaoweizhen/pulseguardian/commit/
> c13b63c0b2f226b577a99b29699963a22d72de1d

Comment 12

[Mark Côté [:mcote]]

Heya, if you're comfortable with 'git rebase' you can work on other commits as well, and when I review them you can use rebase to go back and edit the old commit.  That way you'll have a set of commits that we can land all at once.  This is the way I've started working, and it's nice because you can see and test the entire fix at once, but it's easier to review because the work has been split up into logical pieces.

Or if you'd prefer to just work on one commit at a time, we can do that too.  Let me know. :)

Comment 13

[Mike Yao]

Thanks Mark. I will look into 'git rebase', it's the chance to learn more git. 

Working on UI the second phrase.

(In reply to Mark Côté [:mcote] from comment #12)
> Heya, if you're comfortable with 'git rebase' you can work on other commits
> as well, and when I review them you can use rebase to go back and edit the
> old commit.  That way you'll have a set of commits that we can land all at
> once.  This is the way I've started working, and it's nice because you can
> see and test the entire fix at once, but it's easier to review because the
> work has been split up into logical pieces.
> 
> Or if you'd prefer to just work on one commit at a time, we can do that too.
> Let me know. :)

Comment 14

[Mike Yao]

The work are almost done. There are two questions:

* I can't find script to send emails? My understanding is: 

there's "emails" property on the class "PulseGuardian", the code is like "PulseGuardian(api, emails)", can't find any code like this.

* For the autosuggest, could it be separated to another ticket? I need to think about the UI. 

Sorry about response on late.

Comment 15

[Mike Yao]

The models are separated to each file. There's import problem when complex relationships among models. So Could some of models be merged?

Comment 16

[Mark Côté [:mcote]]

(In reply to Mike Yao from comment #14)
> The work are almost done. There are two questions:
> 
> * I can't find script to send emails? My understanding is: 
> 
> there's "emails" property on the class "PulseGuardian", the code is like
> "PulseGuardian(api, emails)", can't find any code like this.

The emails are sent in PulseGuardian.warning_email() and PulseGuardian.deletion_email() in pulseguardian/guardian.py, which both call the sendemail() function from pulseguardian/sendemail.py.  At the moment the 'user' object is passed to both those functions, and an email is only sent to user.email.  You'll have to change the arguments to these functions.

> * For the autosuggest, could it be separated to another ticket? I need to
> think about the UI. 

Yup, that'd be fine!

(In reply to Mike Yao from comment #15)
> The models are separated to each file. There's import problem when complex
> relationships among models. So Could some of models be merged?

Sure, you could even merge them all into a single file since there aren't very many.

Comment 17

[Mike Yao]

The code was committed but the log was not clean. The first three is my work. Is there way to fix that?

https://github.com/yaoweizhen/pulseguardian/commits/master

On the queue list page, the auto refreshing was switched off. It will be hard to input anything. 

How do you try my code? Checkout my reponsitory or send pull request?

Thanks.

Comment 18

[Mark Côté [:mcote]]

(In reply to Mike Yao from comment #17)

Hi, thanks for the patches!  I will review them tomorrow.

> The code was committed but the log was not clean. The first three is my
> work. Is there way to fix that?

What do you mean by "the log was not clean"?  As in the commit history isn't clean?  If so then you can use git's rebase command to edit and squash together commits.  Also when you get familiar with the rebase command, I recommend splitting your commits up into smaller pieces that build toward the new feature.  For example, if you have to refactor something before you can build on it, put the refactor as one commit, and the new stuff as another.  It makes the code both easier to test and easier to review.  We're trying to do that more and more at Mozilla (the new code-review tool, MozReview, is built around that).

I also suggest developing in a separate branch.  It'll make it easier to keep in sync with the main repo (you should rebase your changes on the latest master).

> On the queue list page, the auto refreshing was switched off. It will be
> hard to input anything. 

Hm we should come up with a solution for that, perhaps doing the refreshing without reloading the whole page.

> How do you try my code? Checkout my reponsitory or send pull request?

Not super important, but a pull request is probably easiest.

Comment 19

[Mark Côté [:mcote]]

Hi again.  So first thing, there seems to be a problem in the UI.  If I have multiple queues and I add a notification to one queue, it appears on both.  Refreshing it causes it to appear on the correct queue, though, and in the database it's only associated with the one queue.

Second, I see that you are using the queue name in email_notifications because the queue table doesn't have an ID column.  It would be better if you could add an ID to the queues and use that (not sure why it didn't have one to begin with, to be honest).

You're also missing a few imports in your new models.py, which, for example, causes an error if you try to change a PulseUser's password.

Finally, and most importantly... you don't actually seem to be sending out notifications to the addresses in queue_notifications. :)

Let me know if you have any questions.

Comment 20

[Mike Yao]

> 
> What do you mean by "the log was not clean"?  As in the commit history isn't
> clean?  If so then you can use git's rebase command to edit and squash
> together commits.  Also when you get familiar with the rebase command, I
> recommend splitting your commits up into smaller pieces that build toward
> the new feature.  For example, if you have to refactor something before you
> can build on it, put the refactor as one commit, and the new stuff as
> another.  It makes the code both easier to test and easier to review.  We're
> trying to do that more and more at Mozilla (the new code-review tool,
> MozReview, is built around that).

I means rebase only works for local commits, but if the commits pushed, how to re-organized them?

Thanks.

Comment 21

[Mike Yao]

Thanks for your pointing. I will fix that.

(In reply to Mark Côté [:mcote] from comment #19)
> Hi again.  So first thing, there seems to be a problem in the UI.  If I have
> multiple queues and I add a notification to one queue, it appears on both. 
> Refreshing it causes it to appear on the correct queue, though, and in the
> database it's only associated with the one queue.
> 
> Second, I see that you are using the queue name in email_notifications
> because the queue table doesn't have an ID column.  It would be better if
> you could add an ID to the queues and use that (not sure why it didn't have
> one to begin with, to be honest).
> 
> You're also missing a few imports in your new models.py, which, for example,
> causes an error if you try to change a PulseUser's password.
> 
> Finally, and most importantly... you don't actually seem to be sending out
> notifications to the addresses in queue_notifications. :)
> 
> Let me know if you have any questions.

Comment 22

[Mark Côté [:mcote]]

(In reply to Mike Yao from comment #20)
> > 
> > What do you mean by "the log was not clean"?  As in the commit history isn't
> > clean?  If so then you can use git's rebase command to edit and squash
> > together commits.  Also when you get familiar with the rebase command, I
> > recommend splitting your commits up into smaller pieces that build toward
> > the new feature.  For example, if you have to refactor something before you
> > can build on it, put the refactor as one commit, and the new stuff as
> > another.  It makes the code both easier to test and easier to review.  We're
> > trying to do that more and more at Mozilla (the new code-review tool,
> > MozReview, is built around that).
> 
> I means rebase only works for local commits, but if the commits pushed, how
> to re-organized them?
> 
> Thanks.

You'll have to use "push -f".  That will update the push request itself.  It's sometimes not a good idea, like if you are working on a branch that others have pulled from, but for work in progress like this it's okay.

Comment 23

[Mark Côté [:mcote]]

Saw your updates on GitHub.  Getting close!  I think there's only one real bug left, in _sendemail(), plus you need to fix the merge problems in the last commit.  Thanks for continuing with this!  It's going to be really useful. :)

Comment 24

[Mark Côté [:mcote]]

I was thinking that we could solve the autonotification problem in one of two ways:

a) Put the display logic on the client side and only load JSON data instead of all the HTML, or
b) Make the notification input a modal dialog instead of inline.

(a) is more work but is more consistent with other input fields in PulseGuardian, plus we'd get a useful API "for free".

Comment 25

[Mark Côté [:mcote]]

Very close. :) I have a number of comments, but the biggest problem is that it crashes when actually trying to email the notification addresses.

When I'm testing, I set the messages thresholds to be very small so that I just need to publish a small number of messages to trigger an email.  I also set the polling interval to a very small value.  You could try that too, e.g. with

export WARN_QUEUE_SIZE=5
export DEL_QUEUE_SIZE=10
export POLLING_INTERVAL=5

Anyway, almost there, and most of the code is very good! :)

Comment 26

[Bob Clary [:bc] (inactive)]

If we can get this over the finish line I'm sure many of us would be quick to make use of it.

Comment 27

[Mike Yao]

Sorry guys, let me try to get it done on this weekend.

(In reply to Bob Clary [:bc:] from comment #26)
> If we can get this over the finish line I'm sure many of us would be quick
> to make use of it.

Comment 28

[Mark Côté [:mcote]]

I was thinking more about this, and making this a modal dialog is *much* easier than converting all the display logic to be client side.  Mike, do you mind trying that?  Looks like it's pretty easy with Bootstrap: http://getbootstrap.com/javascript/#modals.  I don't think you'd even need to suspend the autoreload.

Comment 29

[Mike Yao]

(In reply to Mark Côté [:mcote] from comment #28)
> I was thinking more about this, and making this a modal dialog is *much*
> easier than converting all the display logic to be client side.  Mike, do
> you mind trying that?  Looks like it's pretty easy with Bootstrap:
> http://getbootstrap.com/javascript/#modals.  I don't think you'd even need
> to suspend the autoreload.

You mean use modal dialog instead of notification form attaches to each queue?

If the content is reloaded frequently, it can not do anything. So easy way is keeping autoreload, disable it when need to add notifications, then enable back. 

If autoreload must be, the way is reloading json on background and then update content besides notfication form. The new queues will be appended to page.

Comment 30

[Mark Côté [:mcote]]

(In reply to Mike Yao from comment #29)
> You mean use modal dialog instead of notification form attaches to each
> queue?
> 
> If the content is reloaded frequently, it can not do anything. So easy way
> is keeping autoreload, disable it when need to add notifications, then
> enable back. 

Yes, instead of putting the form right into the queue-listing html.  It isn't very user friendly to require the person to manually disable autoreload, and I also don't think it'd be very easy to automatically disable it in a graceful way.  A modal dialog solves this.  You click an "add notification" button of some sort for a particular queue, then a dialog pops up requesting the email address to add (and listing the queue name in again, to make it clear).  We wouldn't have to disable autoreload, since the refresh wouldn't affect the dialog (or, if we had to disable autoreload, we could do that automatically when the dialog is opened and then automatically re-enable it when the dialog is closed.  But hopefully we don't have to do that).

> If autoreload must be, the way is reloading json on background and then
> update content besides notfication form. The new queues will be appended to
> page.

Yeah, that's a lot harder though, and would require restructuring how we do everything.  I played around with it a few months ago, and it's a lot of work: first creating the JSON APIs, then writing the front-end code.  It's not a bad idea, but I think it could be done later.  A modal dialog isn't the best solution, but it seems like the fastest way to get this feature finished without breaking autoreload and without requiring the front end to be partially rewritten.

Comment 31

[Mike Yao]

(In reply to Mark Côté [:mcote] from comment #30)
> (In reply to Mike Yao from comment #29)
> > You mean use modal dialog instead of notification form attaches to each
> > queue?
> > 
> > If the content is reloaded frequently, it can not do anything. So easy way
> > is keeping autoreload, disable it when need to add notifications, then
> > enable back. 
> 
> Yes, instead of putting the form right into the queue-listing html.  It
> isn't very user friendly to require the person to manually disable
> autoreload, and I also don't think it'd be very easy to automatically
> disable it in a graceful way.  A modal dialog solves this.  You click an
> "add notification" button of some sort for a particular queue, then a dialog
> pops up requesting the email address to add (and listing the queue name in
> again, to make it clear).  We wouldn't have to disable autoreload, since the
> refresh wouldn't affect the dialog (or, if we had to disable autoreload, we
> could do that automatically when the dialog is opened and then automatically
> re-enable it when the dialog is closed.  But hopefully we don't have to do
> that).
> 
> > If autoreload must be, the way is reloading json on background and then
> > update content besides notfication form. The new queues will be appended to
> > page.
> 
> Yeah, that's a lot harder though, and would require restructuring how we do
> everything.  I played around with it a few months ago, and it's a lot of
> work: first creating the JSON APIs, then writing the front-end code.  It's
> not a bad idea, but I think it could be done later.  A modal dialog isn't
> the best solution, but it seems like the fastest way to get this feature
> finished without breaking autoreload and without requiring the front end to
> be partially rewritten.

Ok, let's go modal dialog solution. The "Add notification" button might be a little bit hard to click on :).

Comment 32

[Mike Yao]

Phew, the modal dialog was implemented but there's extra click when adding notification each time.

Thanks Mark. 

https://github.com/yaoweizhen/pulseguardian/commits/notification

Comment 33

[Mark Côté [:mcote]]

Sorry for the delay in reviewing this.  I'm going to get to it soon.

Comment 34

[Mark Côté [:mcote]]

Sorry again for this lingering on.  One big thing is that the work is based on a pretty old revision and will need to be rebased.  If you happen to be still interested in doing this, please go ahead; otherwise I'll try to find the time to fix this up myself.

Comment 35

[Mike Yao]

(In reply to Mark Côté [:mcote] from comment #34)
> Sorry again for this lingering on.  One big thing is that the work is based
> on a pretty old revision and will need to be rebased.  If you happen to be
> still interested in doing this, please go ahead; otherwise I'll try to find
> the time to fix this up myself.

Ok, I can do it.

Comment 36

[Mark Côté [:mcote]]

Unassigning due to lack of activity.

Comment 37

[Dustin J. Mitchell [:dustin] (he/him)]

What we want is a queue which can be shared between users, so that we all are getting those messages. As of now this doesn't seem to be possible with PulseGuardian. Maybe Marc can comment on that.

I think this original request is now met: multiple people can now be associated with a Pulse user. Bug 1071947 might be the next step, but that's .. another bug :)