496419 - Managing permissions from other accounts confusing

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Collections, defect, P2)

Description

[Justin Scott [:fligtar]]

I created some collections that I wanted to transfer ownership of to my other account. I added the other account as a manager, but when I logged into that account, I saw the attached screen.

The "myself" there is still referring to the original owner, it seems, and my email address was the only one listed.

So, in editing permissions, "myself" should always be the currently logged in user, and the list of people with permissions should be everyone except "myself".

Comment 1

[Justin Scott [:fligtar]]

Attachment: screenshot

Comment 2

[Fred Wenzel [:wenzel]]

Hm, that does make sense. But it is also a big change of the page logic probably including string changes. Is this a blocker for 5.0.6?

Comment 3

[Justin Scott [:fligtar]]

I'm not sure what string changes would be needed. But I suppose it's not a blocker.

Comment 4

[Fred Wenzel [:wenzel]]

(In reply to comment #3)
> I'm not sure what string changes would be needed.

Because "myself" refers to the owner. So far, the owner is never shown in any of these lists (so that a collection can never be "owner-less"). Remember that the owner or an admin are the only people who can delete a collection. So if we want to be able to give collections to someone else as a gift, we need slightly different UI, allowing to promote a manager to an owner.

So the easiest fix would be showing "myself" for the owner, and "the owner" for all managers who can also edit permissions. But that's a string change :-/

Comment 5

[Justin Scott [:fligtar]]

I didn't realize there was a difference between owner and manager. I thought there were only 2 possible roles.

Yeah, we'll bump this to 5.0.7 then.

Comment 6

[Justin Scott [:fligtar]]

So, to clarify, this is how I think we should fix this bug:

There are only 2 roles on a collection: owner/manager and editor. The person that creates a collection is an owner and can be removed by another owner, just like how add-on roles work. The permission settings in Manage Collection should be relative to the logged-in user, such that "myself" actually does refer to the person logged in.

Comment 7

[Fred Wenzel [:wenzel]]

(In reply to comment #6)
> There are only 2 roles on a collection: owner/manager and editor.

Hm. Both the API an public pages distinguish between owner and manager and thus know 3.

Comment 8

[Fred Wenzel [:wenzel]]

(In reply to comment #7)
> (In reply to comment #6)
> > There are only 2 roles on a collection: owner/manager and editor.
> 
> Hm. Both the API an public pages distinguish between owner and manager and thus
> know 3.

CCing lorchard who wrote the initial code, I think.

Comment 9

[Les Orchard [:lorchard]]

Not sure, but I think rdoherty wrote the original code for roles in collections models.

Comment 10

[Fred Wenzel [:wenzel]]

Something else came to my mind: The biggest point currently distinguishing an owner from a manager is, that the owner can delete a collection, while a manager cannot.

Comment 11

[Ryan Doherty (:rdoherty)]

I relied on what I thought were the roles for add-ons in constants.php:

define('AUTHOR_ROLE_ADMINOWNER', 7);
define('AUTHOR_ROLE_ADMIN', 6);
define('AUTHOR_ROLE_OWNER', 5);
define('AUTHOR_ROLE_DEV', 4);
define('AUTHOR_ROLE_VIEWER', 1);
define('AUTHOR_ROLE_NONE', 0);

I didn't do too much after that really, almost all the collections code has
been re-written since I worked on it.

Comment 12

[Fred Wenzel [:wenzel]]

Fligtar: I think we need some clarification here.

Currently, managers and the owner are two different roles. The owner cannot be removed and is (besides admins) the only person to delete an entire collection.

If we want to change this, we should be aware of the implications (yes, a manager can kick you out of your own collection), then fix it according to the new requirements.

Comment 13

[Justin Scott [:fligtar]]

There should only be 2 roles, as per the spec (W-1.4.2). I think Ryan was confused by the add-on author roles, where there is an "admin" but that refers to a site admin, not an add-on admin role.

There is one thing on the site that add-on authors can do that admins can't do, and that is mark their add-on statistics dashboard as public. Which is why there had to be an ADMINOWNER role so that someone who is an admin but also the owner of an add-on could mark their own add-on stats dashboard public. But I digress.

I would like to be able to just let this stay as-is, but there needs to be a way to transfer collections and we need to clear up the "myself" issue, and I think the least confusing way to do this is to go back to 2 roles as originally intended.

I am fine with the implications of someone you make a manager kicking you out -- the same is true of the add-ons permission system.

Comment 14

[Fred Wenzel [:wenzel]]

Attachment: SQL demoting owners

First things first: We need to run this SQL after the (upcoming) code lands, in order to "demote" all owners to regular, boring, managers.

Comment 15

[Fred Wenzel [:wenzel]]

(In reply to comment #13)
> There should only be 2 roles, as per the spec (W-1.4.2).

All right, I'll do this ;) Justin, please make sure with the Briks guys that they are not using the author role "owner" anywhere.

Comment 16

[Fred Wenzel [:wenzel]]

Another side note (a corollary, if you will), there won't be a way to see who initially created a collection anymore.

Comment 17

[Fred Wenzel [:wenzel]]

Attachment: Remove collection owner role

This patch removes the collection owner role altogether. It doesn't fix the UI yet, but with this patch, there won't be any owner anymore, they are just managers.

Comment 18

[Fred Wenzel [:wenzel]]

Attachment: Make "myself" refer to the current user only

This patch (based on the previous one) makes the word "myself" properly refer to the current user only, to soothe the newly-demoted owners' pain.

Comment 19

[Wil Clouser [:clouserw]]

These work.  If an admin loads the page the "who can manage my collection" still says "only me" and it doesn't refer to the admin, but I don't think we need to care about that.

Comment 20

[Fred Wenzel [:wenzel]]

Committed to r28256 and r28257. I added the SQL to the "schematic" migration dir in r28261.