[PulseGuardian] Allow multiple Users per PulseUser

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: mcote, Assigned: camd)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
I can easily see situations in which someone has registered a service under their name, and at some point they are no longer responsible for that service.  PulseGuardian should have a way to transfer a user by allowing user A to send an invite to user B, who can reject or accept the offer.
(Reporter)

Updated

4 years ago
Depends on: 1068438

Comment 1

4 years ago
The idea on this is for a user to able to transfers a PulseUser to another other user?
Flags: needinfo?(mcote)
(Reporter)

Comment 2

4 years ago
Thinking about this more, I think what's more useful is to allow a PulseUser to be associated with multiple User (Persona) accounts.  This means that multiple people can get warning emails, in case one is away, and also solves the problem of transfership--a User account can disassociate itself from a PulseUser account if if it still has other associated User accounts.

But I guess the core of this is still relevant... I think it would be up to an existing User account to invite another user to associate itself with that PulseUser.  But the other User would have to accept, so we'd need some sort of notification system (and UI).  Not terribly complicated, but a bit of work.

Updating summary appropriately.
Flags: needinfo?(mcote)
Summary: [PulseGuardian] Allow transfer of Pulse accounts → [PulseGuardian] Allow multiple Users per PulseUser with support for invitations and the ability to disassociate

Comment 3

4 years ago
Sounds good :) I'll start thinking on how we can achieve this.

Updated

4 years ago
Assignee: nobody → mcc.ricardo
Status: NEW → ASSIGNED

Comment 4

4 years ago
An outline for a possible solution. 

Each PulseUser could have more than 1 User. A new functionality would allow invites for new Users. The user could accept or not. 

If accepted, the all Users would receive the normal notifications.

A User could unsubscribe a PulseUser, as long as there are more Users "watching".

Comment 5

4 years ago
An outline for implementation.

New button on each PulseUser would trigger some modal or new page to allow inviting new Users (by email possibly). A new table for "Invites" would be created to hold them.

Each time a user visits the app (either by logging in or coming back) that new table is checked and the User is warned (clickable number next to his email possible?) of how many new invites he has pending. By clicking that number a list of "Invites" with buttons to accept and reject is displayed. If any of them is accepted, it will be added it's PulseUser list.

A new button to "Stop Watching" needs to be added to each PulseUser for each User.

I have to look into the database and check how exactly the current implementation is and to support multiple Users per PulseUser.

What do you think?
Flags: needinfo?(mcote)
(Reporter)

Comment 6

4 years ago
That all sounds great!  The database changes for multiple users per PulseUser shouldn't be difficult; you'd just need another mapping table.  A migration script would be nice though.
Flags: needinfo?(mcote)

Comment 7

4 years ago
Excellent.

Do you prefer an SQL script to handle the migration or should I use something like alembic? (https://pypi.python.org/pypi/alembic)
Flags: needinfo?(mcote)
(Reporter)

Comment 8

4 years ago
Alembic or similar would be great!  We're definitely going to have more schema changes in the future.
Flags: needinfo?(mcote)

Comment 9

4 years ago
Excellent!

Comment 10

4 years ago
We currently have a "One To Many" relationship between User and PulseUser:

> class User:
>    __tablename__ = 'users'
>    id = Column(Integer, primary_key=True)
>    ...
>    pulse_users = relationship(PulseUser, backref='owner',
>                                cascade='save-update, merge, delete')
> 
> class PulseUser:
>    __tablename__ = 'pulse_users'
>    id = Column(Integer, primary_key=True)
>    ...
>    owner_id = Column(Integer, ForeignKey('users.id'), nullable=True)

What we want is to transform that into a "Many To Many" between the two tables, using an association table:

> association_table = Table('association', Base.metadata,
>    Column('users_id', Integer, ForeignKey('users.id')),
>    Column('pulse_users_id', Integer, ForeignKey('pulse_users.id'))
>)
>
> class User:
>    __tablename__ = 'users'
>    id = Column(Integer, primary_key=True)
>    ...
>     pulse_users = relationship(PulseUser, backref='owner',
>                                cascade='save-update, merge, delete', secondary=association_table)
> 
> class PulseUser:
>    __tablename__ = 'users'
>    id = Column(Integer, primary_key=True)
>    ...
Flags: needinfo?(mcote)

Comment 11

4 years ago
Thinking a little bit more about this...

Currently a User can have several PulseUsers. Being a User the only responsible for a PulseUser he can delete it whenever he wants. The overall idea is that several Users can share the same PulseUser. With that in mind:

 - as discussed, we will allow a User to "leave" a PulseUser if he's not the only User responsible for him;
 - but, who can delete PulseUsers? We if implement the changes above, any User can do it. Is that the desired behavior?

If that's the desired behavior (multiple full ownership), I think we can go ahead (after your review of the above) with implementing the changes. If not, we might think about two relationships: 

 - Users that "own" PulseUsers (as it is right now)
 - the concept of Users "observing" PulserUsers. That way only the "owner" can delete it's own PulserUsers but can be an "observer" of multiple.

What do you think?
(Reporter)

Comment 12

4 years ago
Good questions!

The main problem this feature is trying to solve is supervising Pulse users when the original owner is unavailable (on PTO, no longer a contributor, etc.).  With this in mind, it makes sense to grant full ownership to the other users.  It's also the simplest approach, which I like. :)  So yes, let's go for fully shared ownership; if we later want to add the idea of an "observing" user (an interesting idea!) we can add it then.  I am a big fan of YAGNI[1]. :)

And yes, the association table (which I tend to call a "mapping" table, but that's just my own personal preference) is what we want.  I was thinking we'd need a specific model for it, but I'm not super-well-acquainted with sqlalchemy, so whatever is the standard approach. :) We just need to name it something descriptive.  "pulse_user_owners" or "pulse_user_owner_map" or something like that.  Something that indicates the specific purpose of the table.

[1] http://c2.com/cgi/wiki?YouArentGonnaNeedIt
Flags: needinfo?(mcote)

Comment 13

4 years ago
Absolutely! It's definitely the simplest approach. And of course, if we don't need it there's no point :D Also a big fan of YAGNI!

I'm not very-acquainted with sqlalchemy. Let me check the docs for the standard approach.

Comment 15

3 years ago
So, what will do will be the following:

> pulse_user_owners = Table('pulse_user_owners', Base.metadata,
>    Column('users_id', Integer, ForeignKey('users.id')),
>    Column('pulse_users_id', Integer, ForeignKey('pulse_users.id'))
>)
> 
> class User:
>    __tablename__ = 'users'
>    id = Column(Integer, primary_key=True)
>    ...
>     pulse_users = relationship(PulseUser, backref='owner',
>                                cascade='save-update, merge, delete', secondary=association_table)
> 
> class PulseUser:
>    __tablename__ = 'users'
>    id = Column(Integer, primary_key=True)
>    ...

I'll figure out how to use Alembic to migrate the database, creating that new table. 

Also, we need to update tests and dbinit.py. Should I create 2 new bugs that block this one?
Flags: needinfo?(mcote)
(Reporter)

Comment 16

3 years ago
If they are all logically related, which it sounds like they are, then using the same bug is best.  You can split them into multiple commits in a single pull request, if you like, which can make reviewing easier.  The usual criteria for splitting into several commits is that they should be able to stand alone (nothing would break if an earlier commit was landed and deployed before a later one) but they are all working together to solve a specific problem.  Or in other words, "Each commit should be as small as possible, but no smaller".  Hope that makes sense. :)
Flags: needinfo?(mcote)
(Reporter)

Comment 17

3 years ago
Bumping this up as another person has requested it.
Priority: P3 → P1

Comment 18

3 years ago
Starting to look into this again. Sorry for the delay.
(Reporter)

Comment 19

3 years ago
No problem, and thanks!

Comment 20

3 years ago
Created attachment 8601347 [details]
DB after migration script.

Added migration (upgrade an downgrade) script. DB after migration looks like in the picture.
(Reporter)

Comment 21

3 years ago
Looks good!

Comment 22

3 years ago
Created attachment 8602557 [details]
DB after second migration.

Created 'invites' table.

Comment 23

3 years ago
Hi Mark,

Just added the fucntionality to invite a User when a PulseUser is created. The invite is done in that screen by adding a list (coma separated strings) of emails to a new form field. Do you want to add some "error message" on PulseUser creation in case any of those Users does not exist?
Flags: needinfo?(mcote)
(Reporter)

Comment 24

3 years ago
Yeah, that's probably a good idea.
Flags: needinfo?(mcote)

Comment 25

3 years ago
Cool! Since 'invitations' are optional, I'll be adding a warn message to the profile page once the user get redirected.

Comment 26

3 years ago
Hi Mark,

I was having some trouble updating 'all_pulse_users' but I got somewhere. That being said, I have an issue and I need some help with it.

Currently, 'all_pulse_users' looks like this: https://github.com/mccricardo/pulseguardian/blob/master/pulseguardian/web.py#L199

I made an update and so far it's something like this:

> def all_pulse_users():
>     users = db_session.query(
>         PulseUser.username,
>         case([(User.email == None, "None")], else_=User.email)).join(
>             User.pulse_users).all()
>     return render_template('all_pulse_users.html', users=users)

The issue with this is that if a PulseUser has multiple Users there will be multiple lines for each PulseUser. For example:

> fake_user 	fake@email.com
> fake_user 	fake2@email.com

Wouldn't it be preferable to have something like:

> fake_user 	fake@email.com, fake2@email.com

And if so, how can I achieve that?
Flags: needinfo?(mcote)
(Reporter)

Comment 27

3 years ago
(In reply to Ricardo Castro from comment #26)
> The issue with this is that if a PulseUser has multiple Users there will be
> multiple lines for each PulseUser. For example:
> 
> > fake_user 	fake@email.com
> > fake_user 	fake2@email.com
> 
> Wouldn't it be preferable to have something like:
> 
> > fake_user 	fake@email.com, fake2@email.com
> 
> And if so, how can I achieve that?

Yes, the latter would be preferable.  There are probably a few different ways to do this.  I am not sure if there's a fancy way to do this directly in sqlalchemy, but you could use the query as it is now but combine them afterward, in the all_pulse_users() function, before rendering the template.

What you're going to want is a list of tuples like we have now, except the first items (pulse_user) in the tuple will occur only once each, and the second item (currently called "user" in the template, but that should probably be changed, say, to "owners") would be the list of all the matching User.emails, join(", ")-ed together.  Probably easiest to construct a dictionary first, of the form {'fake_user': ['fake@email.com', 'fake2@email.com'], ...} and then turn that into the list of tuples [('fake_user', 'fake@email.com, fake2@email.com'), ...] to pass to the template.

Don't forget to sort the owner list for each pulse user before combining them the string. :)
Flags: needinfo?(mcote)

Comment 28

3 years ago
Thanks Mark. That sounds good!

I though about somethign along these lines but decided to ask first in case you had some other ideas. 

Thanks!

Comment 29

3 years ago
A quick update on my (slow!) progress. So far I have:

 - migrations
 - updated to add invites on pulse_user creation
 - site header with invite count (it being a link)
 - page with invites list, with buttons to accept or reject invite
 - accepting or rejecting invites already implemented
 - listing all owners (ordered) for each pulse_user

Still to be done:

 - button do stop following a pulse_user (only allowing dissociation if there's more than one owner)
 - tests update
 - update pulse_user removal (since the schema was updated, delete is broken)

Work is being tracked here: https://github.com/mccricardo/pulseguardian/tree/multiple-users-per-pulseuser]
(Reporter)

Comment 30

3 years ago
Great! :)
\o/
Thanks, Ricardo!

Comment 32

3 years ago
I was doing soem testing and realized the following that dbinit.py and our migrations will only work for existing db's. This happens because if I'm starting from scratch usgin dbinit.py it will create one of the tables that I was managing via the migrations.

I'm thinking about creating an initial migration that will create the initial, existing, schema. What do you think?
Flags: needinfo?(mcote)
(Reporter)

Comment 33

3 years ago
Yes, the dbinit.py was basically a hack.  Using a standardized way to set up a database would be much preferred.

Also, I was thinking that now might be a good time to post some screenshots so you could get some feedback on UI, if that makes sense to you.

Thanks for sticking with this!
Flags: needinfo?(mcote)
(Reporter)

Comment 34

2 years ago
I'm unassigning this due to inactivity, to free it up if anyone else wants to take it over the finish line.
Assignee: mcc.ricardo → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 35

2 years ago
I'll take this for now.  I created a branch from Ricardo's based on latest master and pushed it up.  I'm going to try to address this soon.
(Assignee)

Updated

2 years ago
Assignee: nobody → cdawson
(Assignee)

Comment 36

2 years ago
new branch is: multiple-user-ownership
Can we just skip the invitations feature?

As an MVP, I would naively think all we need is:
* Schema changes to support a many to many mapping of owners to pulse usernames
* UI changes to display which owners are associated with a username
* For individuals that own a username, UI that is displayed that allows them to immediately add other owners to that username (with no invite process), as well as delete other owners

I don't think we need:
* invites
* any different level of ownership (ie owner vs manager)
(Reporter)

Comment 38

2 years ago
Yeah I actually thought I had mentioned that the invite feature wasn't super important, but apparently I didn't. :) And it's even less important once we restrict membership via Okta.

And yes, only one level of ownership is fine.
(Assignee)

Comment 39

2 years ago
Created attachment 8831229 [details] [review]
First PR add shared pulse-user ownership

This is the first PR that does almost everything.  The second one will remove the unused single-owner field and remove the Foreign key.
Attachment #8831229 - Flags: review?(mcote)
(Reporter)

Comment 40

2 years ago
Comment on attachment 8831229 [details] [review]
First PR add shared pulse-user ownership

As per 1:1, clearing review.
Attachment #8831229 - Flags: review?(mcote)
(Assignee)

Comment 41

2 years ago
Comment on attachment 8831229 [details] [review]
First PR add shared pulse-user ownership

OK, I broke this down into tasty little morsels.  Hopefully it's a bit more palatable now.  :)
Attachment #8831229 - Flags: review?(mcote)
(Assignee)

Comment 42

2 years ago
Created attachment 8831897 [details] [review]
Second PR add shared pulse-user ownership

This code branches off the first PR branch.  So only the very last commit needs reviewing.
Attachment #8831897 - Flags: review?(mcote)
(Reporter)

Comment 43

2 years ago
Comment on attachment 8831229 [details] [review]
First PR add shared pulse-user ownership

Comments left in PR.  A few things to fix, mainly for clarity.
Attachment #8831229 - Flags: review?(mcote) → review-
(Assignee)

Comment 44

2 years ago
Comment on attachment 8831229 [details] [review]
First PR add shared pulse-user ownership

OK, back to you, Boss..  :)
Attachment #8831229 - Flags: review- → review?(mcote)
(Reporter)

Comment 45

2 years ago
Comment on attachment 8831229 [details] [review]
First PR add shared pulse-user ownership

Looks good!
Attachment #8831229 - Flags: review?(mcote) → review+
(Reporter)

Comment 46

2 years ago
Comment on attachment 8831897 [details] [review]
Second PR add shared pulse-user ownership

Clearing review until my question in the PR is resolved.
Attachment #8831897 - Flags: review?(mcote)
(Assignee)

Comment 47

2 years ago
Comment on attachment 8831897 [details] [review]
Second PR add shared pulse-user ownership

Thanks for catching that removal of ``owner_id``.  Fixed now.  Things look good up on PG, so this one is ready to go now, too.
Attachment #8831897 - Flags: review?(mcote)
(Reporter)

Updated

2 years ago
Attachment #8831897 - Flags: review?(mcote) → review+

Updated

a year ago
Blocks: 1347162
(Reporter)

Updated

a year ago
Summary: [PulseGuardian] Allow multiple Users per PulseUser with support for invitations and the ability to disassociate → [PulseGuardian] Allow multiple Users per PulseUser
(Assignee)

Comment 48

a year ago
Merged second PR and ready for prod deploy.  Will do that today.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.