456132 - [W-1.4] Collection Management

Status: VERIFIED FIXED

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

Description

[Justin Scott [:fligtar]]

From the specs:
    *  When logged in as a user with collections, a Collection Management area will list the collections, the number of subscribers, and have a link to edit the collection or delete it.
    * The Collection Edit page will allow changing all of the fields created at creation except for the ID.

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

Attachment: draft 1: feeds view

Feeds view: User is already logged in, and is viewing the feeds that they manage.  This draft has a "slinky-style," like Fashion Your Firefox, which means that all administrative actions happen on this page.

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

Attachment: draft 1: password protect

User protects his feed with a password

Comment 3

[Jennifer Morrow [:Boriss] (UX)]

Attachment: draft 1: restrict to email addresses

User makes his feed only able to certain email addresses (where email addresses could be bandwagon logins, AMO logins, etc)

Comment 4

[Jennifer Morrow [:Boriss] (UX)]

Attachment: draft 1: deletiong a feed

User deletes a feed

Comment 5

[Wil Clouser [:clouserw]]

There needs to be parity with the creation form.  I don't see a way to edit: name, description, who can edit, or the categories


> Created an attachment (id=353043) [details]
> draft 1: password protect
I think the password boxes should appear directly below "Who can view this feed"

> Created an attachment (id=353044) [details]
> draft 1: restrict to email addresses
- There should be a way to add/remove in bulk.  Accepting a list of comma or space separated values is fine.
- This page will need explanation for what exactly is happening and what addresses I can add.  In fact, is there documentation on that so I can read it?

> Created an attachment (id=353045) [details]
> draft 1: deletiong a feed
How long can a user undo a deletion and how long does the message stay on their screen (i.e. if they navigate away is the option to undo lost?)?

Comment 6

[Justin Scott [:fligtar]]

Feeds view feedback:
--------------------
* This page looks good. I like the listing of "my collections"

Editing a feed:
---------------
* I agree with Wil that the page for editing a feed should be similar to the one used to create it, allowing editing for all fields except the id. I see that you put the write password part that I just mentioned in the Collections Creation bug here, which is fine, but it also should be in the creation form, because for almost all cases, the user will not want the feed to be publicly writable by default.

Making feed available to certain email addresses:
-------------------------------------------------
* This feature isn't in the specs and can't be easily supported. The Bandwagon extension doesn't authenticate to a specific AMO account (intentionally), so we can't grant granular permissions like that yet.

Deleting a feed:
----------------
* I think having an undo option will be considerably more implementation effort than it's worth.

Comment 7

[Nick Nguyen [:osunick]]

According to the spec,

http://docs.google.com/Doc?id=dds6vwb4_13fcmxngfd&invite=fgz9ppb

You can also modify feeds which are open to editing by the public.  So I might change the nomenclature to encompass feeds the user has edited or have a mode that shows "my created feeds" vs "feeds i've edited".  

About ID's- I don't think we should rely on user generated ID's, we should generate these internally- users hate having to meet uniqueness requirements.

Comment 8

[Justin Scott [:fligtar]]

(In reply to comment #7)
> You can also modify feeds which are open to editing by the public.

If that was in the spec, it was unintentional (and I can't find it there now). "Open" feeds can only be written to by the public -- their properties cannot be modified by anyone except the AMO account that created it.

Comment 9

[Justin Scott [:fligtar]]

With the new spec, the list of the user's "owned" feeds will be moved to a part of the directory, so that aspect isn't needed now. Each feed in that list will have an Edit/Management link that will go to this page, and it's again broken into 3 (tabbed?) sections:

1) Properties/Metadata - user can edit the fields set at creation. The user can also request a unique collection nickname that can be used in addition to the collection ID in a URL.
2) Permissions - can add and remove user roles
3) Add-ons - can mass remove add-ons already in the collection and mass add new add-ons

Comment 10

[Justin Scott [:fligtar]]

->M3.

Comment 11

[Justin Scott [:fligtar]]

Attachment: updated management wireframe, v1

This is my first run at the management wireframe updated with the latest spec. I'm not in love with a few parts, but interested to hear what other people think.

Comment 12

[Brian King [:kinger]]

Associated application Advanced - shouldn't that be a multi-select?

Comment 13

[Wil Clouser [:clouserw]]

I like it

Comment 14

[Justin Scott [:fligtar]]

(In reply to comment #12)
> Associated application Advanced - shouldn't that be a multi-select?

We're thinking each collection should only be associated with one application.

Comment 15

[Nick Nguyen [:osunick]]

I think phase 1 is single application, existence of a phase 2 will be dependent on user feedback or more use cases that reveal their existence to us after we launch.

Comment 16

[Nick Nguyen [:osunick]]

wireframe looks fine to me- wondering if "add-ons" should be the second tab but I don't feel strongly about it.

Comment 17

[Justin Scott [:fligtar]]

Okay, Fred, this mockup is ready for implementation when you're available. Again, it's just scaffolding stuff until we get design elements to work with.

Comment 18

[Fred Wenzel [:wenzel]]

What is the interaction with the user lists going to look like (i.e., how do you delete users, for example)? Do we need no-js fallback?

Comment 19

[Justin Scott [:fligtar]]

Moving to revised M5 milestone (4/28).

Comment 20

[Justin Scott [:fligtar]]

Attachment: updated management wireframe, v2

Good point. How about this for JS-enabled and JS-disabled?

Comment 21

[Fred Wenzel [:wenzel]]

Short update on this: I got most of the UI done, minus the AJAX. I'll be working on the PHP next. This bug is humongous.

Comment 22

[Fred Wenzel [:wenzel]]

Attachment: SQL for collection management

Here it comes. First, some SQL.

Comment 23

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 1

90k of collection management, fresh from the oven. Considering the size of the patch, an extensive code review may not be possible. Do you want to look at it a little, then have me set up an instance for QA to look at (or commit it to trunk)?

Comment 24

[Justin Scott [:fligtar]]

Thanks for the patch, Fred!

Could you ask Ryan for review, though? I don't mind reviewing tiny patches that can't hurt anything, but I shouldn't really be reviewing real patches anymore.

Comment 25

[Fred Wenzel [:wenzel]]

Comment on attachment 374320 [details] [diff] [review]
Patch, rev. 1

Sure, no prob.

Comment 26

[Ryan Doherty (:rdoherty)]

We should definitely review all code, even if it's 90k, but maybe we could have check in points next time? Every day or two add a patch with code to be reviewed. 

Wenzel could keep developing while someone reviews the in-progress work and provides feedback. That way someone doesn't have to review a humongous amount of code at once.

Comment 27

[Fred Wenzel [:wenzel]]

All right -- I do have smaller git checkins, but "after the fact", they don't make too much sense individually anymore. While writing, your suggestion would probably have worked well though.

Comment 28

[Justin Scott [:fligtar]]

Ryan, can you review this soon?

Comment 29

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 2

Comment 30

[Fred Wenzel [:wenzel]]

Comment on attachment 375720 [details] [diff] [review]
Patch, rev. 2

Of course, merging the branch into trunk led to major bit-rot, so here's a new patch for you.

Comment 31

[Fred Wenzel [:wenzel]]

Comment on attachment 375720 [details] [diff] [review]
Patch, rev. 2

r?ing Wil as well, so he can take a look if he finds time.

Comment 32

[Fred Wenzel [:wenzel]]

Comment on attachment 375720 [details] [diff] [review]
Patch, rev. 2

Okay, I'll break this down into a few smaller pieces. On the upside, that improves my git skills ;) The remaining patch will still be a little big, but it should be easier to read in coherent chunks.

Comment 33

[Fred Wenzel [:wenzel]]

Attachment: Helper component

First patch: Helper component. This component allows the use of a helper outside a view, inside a controller. This makes MVC cry a little, but it is immensely helpful for us when we need to, for example, use $html->url() in a controller somewhere.

Comment 35

[Fred Wenzel [:wenzel]]

Attachment: Incremental SQL

Here's the incremental SQL, incorporating the changes from bug 491762 as well.

Comment 36

[Fred Wenzel [:wenzel]]

Attachment: SQL changes to repository

These are the same SQL changes, in the SQL schema file.

Comment 37

[Fred Wenzel [:wenzel]]

Attachment: Remove delete scaffolding

Collection deletion is now done as part of the edit page, so we can remove its scaffolding altogether.

Comment 38

[Fred Wenzel [:wenzel]]

Attachment: render collection icons

This patch is straightforward: it is the code for displaying a collection icon. It basically mirrors what we have for addon icons.

Comment 39

[Fred Wenzel [:wenzel]]

Attachment: Actual collections edit patch

This is the core collections management page. Ironically, it's become the same size as the original patch now, but that is because I just added l10n, and there are a huge number of strings. It's therefore desirable that we get them in ASAP so localizers have a chance to work on them.

Also, unlike before, this patch is hopefully easier to understand as it only touches the core of this bug. If you have any questions, please let me know.

Comment 40

[Jeff Balogh (:jbalogh)]

Comment on attachment 376126 [details] [diff] [review]
Helper component

I started to write a test for this, but failed.  There's too much cake in between.

You could move `loadHelper($_helper)` out of the if-else since it's called in both cases.

Comment 41

[Jeff Balogh (:jbalogh)]

Comment on attachment 376134 [details]
Incremental SQL

Some of these changes are already in trunk and attachment 375889 [details], so I don't think the alters would work.

Comment 42

[Jeff Balogh (:jbalogh)]

Comment on attachment 376135 [details] [diff] [review]
SQL changes to repository

Parts of this already exist in trunk.

Comment 43

[Jeff Balogh (:jbalogh)]

Comment on attachment 376150 [details] [diff] [review]
Remove delete scaffolding

I guess this is moving to edit in a future patch?  Deleting_code++

Comment 44

[Jeff Balogh (:jbalogh)]

Comment on attachment 376135 [details] [diff] [review]
SQL changes to repository

Woops, I was looking at a dirty working tree.  This does work.

Comment 45

[Jeff Balogh (:jbalogh)]

Comment on attachment 376134 [details]
Incremental SQL

Ditto.  Sorry 'bout that.

Comment 46

[Fred Wenzel [:wenzel]]

(In reply to comment #40)
> (From update of attachment 376126 [details] [diff] [review])
> I started to write a test for this, but failed.  There's too much cake in
> between.

Yes, it's tragic :-/

> You could move `loadHelper($_helper)` out of the if-else since it's called in
> both cases.

Done. r25339.

Comment 47

[Fred Wenzel [:wenzel]]

The SQL changes are committed to r25340, and the final days of the "delete" scaffolding are r25341.

Comment 48

[Jeff Balogh (:jbalogh)]

Comment on attachment 376157 [details] [diff] [review]
render collection icons

I'm not excited about the duplication here, but you're not the first to go down that road.  It would be a bit unfair (and time-consuming) to ask you to fix it. :)

Comment 49

[Fred Wenzel [:wenzel]]

(In reply to comment #48)
> (From update of attachment 376157 [details] [diff] [review])
> I'm not excited about the duplication here, but you're not the first to go down
> that road.  It would be a bit unfair (and time-consuming) to ask you to fix it.
> :)

Thanks, r25346. If you want, you can file a bug for the cleanup here. At least this way it's not forgotten.

Comment 50

[Fred Wenzel [:wenzel]]

Attachment: Fixing the icon test

I made the sharing API's icon URL absolute, then fixed the respective test.

Comment 51

[Jeff Balogh (:jbalogh)]

Comment on attachment 376187 [details] [diff] [review]
Actual collections edit patch

The good news: it all seems to work.
The bad news: I complain a lot.  Sorry about the length!

Usage notes, not looking at the code:

The whole flash() thing annoys the pants off of me.

When I click "Update Collection" I expect to go to the /edit page I was on, not back to /view.  Is it just me?

The tabs aren't url-addressable.  There should be something like /edit/uuid#permissions to put me on the correct tab.

When I add something from /edit#Add-ons, it flashes "Error saving add-on!" next to "Add to Collection".  That doesn't seem right.

"Note: Comment will appear as though written by original publisher on the original publication date": I'm not sure what that means.

The collection view isn't updated after I press "Update Collection".  Looks like a caching issue since the data is correct from the edit page. -- hmm, still incorrect after flipping debug, which usually clears that stuff.  -- the name updates, but the list of addons and the icon don't -- maybe it's a problem with the listing code?

The limits on name and description are not enforced.  I have a patch for this on bug 491095, but I don't think it will work correctly with the translation shenanigans.

I didn't press "Add" on the Permissions page, so the user I entered wasn't saved.  I didn't know I had to press Add.  Also, hitting enter in that add-user box submits the form without triggering the Add.

Check Availability should have a spinner

I checked the Delete box, forgot about it, made other updates to the collection, and then accidentally deleted my treasured collection.  I was sad.

"Add to Collection" shows "<addon name> [<addon id>]" right before adding my add-on to "Current Add-ons".  It looks weird.

I can't undo a Remove from Current Add-ons.

I have to press Save Comment to get my publisher comments saved.  Then I have to press Update Collection to really save it.

The collection nickname is missing when I go to the edit page, and I can't enter in the same nickname again.  So once you have a nickname, you can't edit again without losing your nickname.

The page looks pretty messed up without js.

There's 36 validation errors.  I think a lot of those come from the translation box, but not all.

I didn't look at the javascript too much, I was tired.

> 
>     function beforeFilter() {
>-        // Disable ACLs because this controller is entirely public.
>-        $this->SimpleAuth->enabled = false;
>-        $this->SimpleAcl->enabled = false;

This makes it hard to get to the /collections page.

>@@ -205,51 +197,200 @@ class CollectionsController extends AppController
>     /**
>      * Edit collection
>      */
>-    function edit($uuid) {
>+    function edit($uuid = null) {

Can this function be broken up?  It's 190 lines right now.

>         $this->Amo->checkLoggedIn(); // must be logged in
>-        $id = $this->Collection->getIdForUUID($uuid);
>-        
>-        if(!empty($this->data)) {
>-            
>-            switch ($this->data['action']) {
>-                case 'Update Collection':
>-                    list($localizedFields, $unlocalizedFields) = $this->Collection->splitLocalizedFields($this->data['Collection']);
> 
>-                    if(!isset($unlocalizedFields['listed'])) {
>-                        $unlocalizedFields['listed'] = 0;
>-                    }
>+        if (empty($uuid)) {
>+            $this->flash(sprintf(_('error_missing_argument'), 'collection_id'), '/', 3);
>+            return;
>+        }

There isn't something lower-level to check this for us?  cake++

>+        $id = $this->Collection->getIdForUuidOrNickname($uuid);
>+        if (!$id) {
>+            $this->flash(_('collection_not_found'), '/', 3);
>+            return;
>+        }
>+        // access rights
>+        $user = $this->Session->read('User');
>+        $writable = $this->Collection->isWritableByUser($id, $user['id']);
>+        $isadmin = $this->SimpleAcl->actionAllowed('Admin', 'EditAnyCollection', $user);
>+        $this->publish('isadmin', $isadmin, false);
>+        if (!$writable && !$isadmin) {
>+            $this->flash(___('error_access_denied'), '/', 3);
>+            return;
>+        }
>+        $this->publish('user', $user);
>+        $role = $this->Collection->getUserRole($id, $user['id']);
>+        $this->publish('role', $role, false);
>+        // some more granular acl distinctions
>+        $atleast_manager = ($isadmin || in_array($role, array(COLLECTION_ROLE_ADMIN, COLLECTION_ROLE_OWNER)));
>+        $this->publish('atleast_manager', $atleast_manager, false);
>+        $atleast_owner = ($isadmin || $role == COLLECTION_ROLE_OWNER);
>+        $this->publish('atleast_owner', $atleast_owner, false);
>+
>+        if (!empty($this->data)) {

Should we check validates()?

>+            // Delete collection?
>+            if (isset($this->data['action']) && $this->data['action'] == 'delete-coll') {
>+                if (!$atleast_owner) {
>+                    $this->flash(___('error_access_denied'), '/', 3);
>+                    return;
>+                }
>+                $this->Collection->delete($id);
>+                $this->flash('Collection successfully deleted!', '/collections/', 3);
>+                return;
>+            }
>+
>+            // regular save
>+            $collectiondata = $this->Amo->filterFields($this->data['Collection'],
>+                array(), array('uuid', 'collection_type', 'access', 'subscribers',
>+                    'created', 'modified', 'downloads'));
>+            list($localizedFields, $unlocalizedFields) = $this->Collection->splitLocalizedFields($collectiondata);
>+            $this->Amo->clean($localizedFields);
>+            $this->Collection->id = $id;
>+            $this->Collection->saveTranslations($id, $this->params['form']['data']['Collection'], $localizedFields);
>+
>+            // handle icon removal/upload
>+            if (!empty($this->data['Icon']['delete'])) {
>+                $unlocalizedFields['icontype'] = '';
>+                $unlocalizedFields['icondata'] = null;
>+            } elseif (!empty($_FILES['icon']['name'])) {
>+                $iconData = $this->Developers->validateIcon($_FILES['icon']);
>+                if (is_array($iconData)) {
>+                    $unlocalizedFields = array_merge($unlocalizedFields, $iconData);
>+                }
>             }
>+  
>+            if (!isset($unlocalizedFields['listed'])) $unlocalizedFields['listed'] = 0;
>+            if ($this->Collection->save($unlocalizedFields)) {
>+                // save noscript data
>+                if ($atleast_manager) {
>+                    // publishers
>+                    if (isset($this->data['Publishers']['p_onlyme']) && $this->data['Publishers']['p_onlyme']) {
>+                        // remove all publishers
>+                        $this->Collection->removeAllUsersByRole($id, COLLECTION_ROLE_PUBLISHER);
>+                    } else {
>+                        // remove old publishers
>+                        if (!empty($this->data['Publishers']['delete'])) {
>+                            foreach ($this->data['Publishers']['delete'] as &$_pid) {
>+                                $this->Collection->removeUser($id, $_pid);
>+                            }
>+                        }
>+                        // save new publishers
>+                        if (!empty($this->data['Publishers']['new'])) {
>+                            $_emails = explode(',', $this->data['Publishers']['new']);
>+                            foreach ($_emails as &$_em) {
>+                                $_em = trim($_em);
>+                                if (empty($_em)) continue;
>+                                $_uid = $this->User->findByEmail($_em, array('id'));
>+                                if (empty($_uid)) continue;
>+                                $this->Collection->addUser($id, $_uid['User']['id'], COLLECTION_ROLE_PUBLISHER);
>+                            }
>+                        }
>+                      }
>+  
>+                    // managers
>+                    if (isset($this->data['Managers']['m_onlyme']) && $this->data['Managers']['m_onlyme']) {
>+                        // remove all managers
>+                        $this->Collection->removeAllUsersByRole($id, COLLECTION_ROLE_ADMIN);
>+                    } else {
>+                        // remove old managers
>+                        if (!empty($this->data['Managers']['delete'])) {
>+                            foreach ($this->data['Managers']['delete'] as &$_pid) {
>+                                $this->Collection->removeUser($id, $_pid);
>+                            }
>+                        }
>+                        // save new managers
>+                        if (!empty($this->data['Managers']['new'])) {
>+                            $_emails = explode(',', $this->data['Managers']['new']);
>+                            foreach ($_emails as &$_em) {
>+                                $_em = trim($_em);
>+                                if (empty($_em)) continue;
>+                                $_uid = $this->User->findByEmail($_em, array('id'));
>+                                if (empty($_uid)) continue;
>+                                $this->Collection->addUser($id, $_uid['User']['id'], COLLECTION_ROLE_ADMIN);
>+                            }
>+                        }
>+                    }

Are the publisher and manager blocks above the same?  They look identical, with s/Publishers/Managers/g and s/PUBLISHER/ADMIN/g.  Can that handling be moved to a helper function?

>+                } // at least manager
>+
>+                // add-ons
>+                // remove old add-ons
>+                if (!empty($this->data['Addons']['delete'])) {
>+                    foreach ($this->data['Addons']['delete'] as &$_aid) {
>+                        $this->AddonCollection->deleteByAddonIdAndCollectionId($_aid, $id, ($atleast_manager ? null : $user['id']));
>+                    }
>+                }
>+                // add new add-ons
>+                if (!empty($this->params['form']['q'])) {
>+                    $_aids = explode(',', $this->params['form']['q']);
>+                    foreach ($_aids as &$_aid) {
>+                        $_aid = trim($_aid); if (empty($_aid)) continue;

Could you do 
if (!empty($_aid))  {
  ...addAddonTo...
}

It reads better than continue;

>+                        $this->Collection->addAddonToCollection($id, $user['id'], $_aid);
>+                    }
>+                }
>+
>+                // grab updated collection nickname/uuid
>+                $updated = $this->Collection->findById($id, array('nickname', 'uuid'));
>+                $this->flash('Collection successfully updated!',
>+                '/collection/'.(empty($updated['Collection']['nickname']) ? $updated['Collection']['uuid'] : $updated['Collection']['nickname']), 3);
>+                return;
>+              }
>+          }
>+
>+        $this->Collection->unbindModel(array('hasAndBelongsToMany'=>array('Addon')));
>+          $collection = $this->Collection->findById($id);
>+          $this->data['Collection'] = $collection;
>+  
>+        // translations
>+        $translations = $this->Collection->getAllTranslations($id);
>+        $this->publish('translations', $translations);
>+
>+        // addons
>+        $addons = $this->AddonCollection->getAddonsFromCollection($id);
>+        $this->publish('addons', $addons);
>+        $addons_noscript = array();
>+        foreach ($addons as &$addon) {
>+            $addons_noscript[$addon['AddonCollection']['addon_id']] = $addon['Addon']['Translation']['name']['string'];
>+          }
>+        $this->publish('addons_noscript', $addons_noscript);

Does the translationbox handle <noscript> well?

>+        }
>+
>+        $this->publish('json', $json, false);
>+        $this->render('json', 'ajax');
>+    }
>+
>+    /**
>+     * AJAX: check if collection nickname is already used
>+     */
>+    function _checkNickname() {

Do you want to checkLoggedIn here?

>+        if (empty($this->params['url']['nickname'])) {
>+            return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'nickname'));
>+        }
>+        $taken = $this->Collection->isNicknameTaken($this->params['url']['nickname']);
>+        return array(
>+            'error' => 0,
>+            'taken' => (int)$taken
>+        );
>+    }
>+
>+    /**
>+     * AJAX: Add / remove user to/from this collection's roles
>+     */
>+    function _handleUser($action) {
>+        if (empty($action)) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'action'));
>+        if (empty($_POST['collection_id'])) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'collection_id'));
>+
>+        $this->Amo->checkLoggedIn(); // must be logged in
>+
>+        $user = $this->Session->read('User');
>+        $role = $this->Collection->getUserRole($_POST['collection_id'], $user['id']);
>+        $isadmin = $this->SimpleAcl->actionAllowed('Admin', 'EditAnyCollection', $user);
>+        $atleast_manager = ($isadmin || in_array($role, array(COLLECTION_ROLE_ADMIN, COLLECTION_ROLE_OWNER)));
>+        if (!$atleast_manager) return $this->Error->getJSONforError(___('error_access_denied'));
>+
>+        switch ($action) {
>+        case 'add':
>+            if (empty($_POST['email'])) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'email'));
>+            $roles = array('publishers' => COLLECTION_ROLE_PUBLISHER, 'managers' => COLLECTION_ROLE_ADMIN);
>+            if (!isset($_POST['role']) || !isset($roles[$_POST['role']]))
>+                return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'role'));
>+
>+            $newuser = $this->User->findByEmail($_POST['email'], array('id'));

Is it safe to be using $_POST?

>+            if (!empty($newuser)) {
>+                if ($this->Collection->getUserRole($_POST['collection_id'], $newuser['User']['id']) === false) {
>+                    $this->Collection->addUser($_POST['collection_id'], $newuser['User']['id'], $roles[$_POST['role']]);
>+                    return array('id' => $newuser['User']['id'], 'email' => $_POST['email']);
>+                } else {
>+                    return $this->Error->getJSONforError('User already exists!');
>+                }
>+            } else {
>+                return $this->Error->getJSONforError(___('error_user_notfound'));
>+            }
>+            break;
>+
>+        case 'del':
>+            if (empty($_POST['user_id']) || !is_numeric($_POST['user_id']))
>+                return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'user_id'));
>+            if ($this->Collection->removeUser($_POST['collection_id'], $_POST['user_id']) !== false) {
>+                return array('id' => $_POST['user_id']);
>+            } else {
>+                return $this->Error->getJSONforError(___('error_user_notfound'));
>+            }
>+            break;
>+
>+        default:
>+            return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'action'));
>+        }
>+    }
>+
>+    /**
>+     * AJAX: Add / remove add-on to/from this collection
>+     */
>+    function _handleAddon($action) {
>+        if (empty($action)) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'action'));
>+        if (empty($_POST['collection_id']) || !is_numeric($_POST['collection_id'])) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'collection_id'));
>+        if (empty($_POST['addon_id']) || !is_numeric($_POST['addon_id'])) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'addon_id'));
>+
>+        $this->Amo->checkLoggedIn(); // must be logged in
>+
>+        $addon_id = $_POST['addon_id'];
>+        $collection_id = $_POST['collection_id'];
>+
>+        $user = $this->Session->read('User');
>+        $writable = $this->Collection->isWritableByUser($collection_id, $user['id']);
>+        $role = $this->Collection->getUserRole($collection_id, $user['id']);
>+        $isadmin = $this->SimpleAcl->actionAllowed('Admin', 'EditAnyCollection', $user);
>+        $atleast_manager = ($isadmin || in_array($role, array(COLLECTION_ROLE_ADMIN, COLLECTION_ROLE_OWNER)));

Can this role stuff be moved to a function?  I think I've seen it a few times already.

>+
>+        if (!$writable && !$isadmin) return $this->Error->getJSONforError(___('error_access_denied'));
>+
>+        switch ($action) {
>+        case 'add':
>+            if ($this->AddonCollection->isAddonInCollection($addon_id, $collection_id))
>+                return $this->Error->getJSONforError('Add-on already exists!');

__()?


>+            $addon = $this->Addon->getAddon($addon_id);
>+            if (empty($addon)) return $this->Error->getJSONforError(___('error_addon_notfound'));
>+            if (false !== $this->Collection->addAddonToCollection($collection_id, $user['id'], $addon_id)) {
>+                return array(
>+                    'id' => $addon_id,
>+                    'name' => $addon['Translation']['name']['string'],
>+                    'iconURL' => $this->Image->getAddonIconURL($addon_id),
>+                    'date' => strftime(_('date'), mktime()),
>+                    'publisher' => $this->Html->linkUserFromModel($user)
>+                );
>+            } else {
>+                return $this->Error->getJSONforError('Error saving add-on!');

___()?

>+            }
>+            break;
>+
>+        case 'del':
>+            if (!$isadmin && !$atleast_manager) {
>+                // publisher's own add-on?
>+                $res = $this->AddonCollection->deleteByAddonIdAndCollectionId($addon_id, $collection_id, $user['id']);
>+            } else {
>+                $res = $this->AddonCollection->deleteByAddonIdAndCollectionId($addon_id, $collection_id);
>+            }
>+            if ($res) {
>+                return array('id' => $addon_id);
>+            } else {
>+                return $this->Error->getJSONforError('Error deleting add-on!');

___()?

> msgid "compatibility_dashboard_center_header"
>diff --git a/site/app/models/addon_collection.php b/site/app/models/addon_collection.php
>index 034b32b..5f1431f 100644
>         if (!is_numeric($collection_id)) return false;
>-        $this->query("DELETE FROM addons_collections WHERE addon_id={$addon_id} AND collection_id={$collection_id}");
>-        return true;
>+        if (!empty($user_id) && !is_numeric($user_id)) return false;
>+
>+        $sql = "DELETE FROM addons_collections WHERE addon_id={$addon_id} AND collection_id={$collection_id}";
>+        if (!empty($user_id)) $sql .= " AND user_id = {$user_id}";
>+
>+        $res = $this->query($sql);
>+        return (!($res === false || $this->getAffectedRows() == 0));
>+    }
>+
>+    /**
>+     * Get a list of add-ons belonging to this collection
>+     *
>+     * @param int $collection_id Collection ID
>+     * @return array list of add-ons
>+     */
>+    function getAddonsFromCollection($collection_id) {
>+        $db =& ConnectionManager::getDataSource($this->useDbConfig);
>+        $collection_id = $db->value($collection_id);

Why do you do this?  I haven't seen that before.

>+        $this->unbindFully();
>+        $res = $this->findAll(array('collection_id' => $collection_id));
>+        if (!empty($res)) {
>+            loadModel('Addon');
>+            loadModel('User');
>+            if (empty($this->Addon)) $this->Addon =& new Addon();
>+            if (empty($this->User)) $this->User =& new User();

These aren't loaded already?

>+            foreach ($res as &$row) {
>+                $row['Addon'] = $this->Addon->getAddon($row['AddonCollection']['addon_id']);
>+                $user = $this->User->findById($row['AddonCollection']['user_id']);
>+                $row['User'] = $user['User'];

clouserw gave me a talkin' to about queries in loops, so <that's going to be trouble>.mp3

>+            }
>+        }
>+        return $res;
>+    }
>+
>+    /**
>+     * set the publisher comment for a specific add-on in a collection (for now,
>+     * en-US only)
>+     *
>+     * @param int $collection_id
>+     * @param int $addon_id
>+     * @param string $comment comment to be saved
>+     * @return bool success
>+     */
>+    function setComment($collection_id, $addon_id, $comment) {
>+        $row = $this->query("SELECT comments FROM addons_collections "
>+            ."WHERE collection_id = {$collection_id} AND addon_id = {$addon_id}");
>+        if (empty($row)) return false; // not found or access denied
>+
>+        $db =& ConnectionManager::getDataSource($this->useDbConfig);
>+        $this->begin(); // transactions are our friends

Yes!

>+
>+        if (!empty($row[0]['addons_collections']['comments'])) {
>+            $id = $row[0]['addons_collections']['comments'];
>+        } else {
>+            // generate a new primary key id
>+            $db->execute('UPDATE translations_seq SET id=LAST_INSERT_ID(id+1);');
>+            $_res = $db->execute('SELECT LAST_INSERT_ID() AS id FROM translations_seq;');
>+            if ($_row = $db->fetchRow()) {
>+                $id = $_row[0]['id'];
>+            } else {
>+                $this->rollback();
>+                return false;
>+            }
>+        }
>+        // delete all existing comments
>+        $db->execute("UPDATE translations SET localized_string = NULL WHERE id = {$id}");

This piece feels like it should be in a model somewhere.  I want to reuse it.

>+
>+        // insert the new one, if applicable (en-US only)
>+        if (!empty($comment)) {
>+            $db->execute("INSERT INTO translations (id, locale, localized_string, created) "
>+                ."VALUES ({$id}, 'en-US', '{$comment}', NOW()) "
>+                ."ON DUPLICATE KEY UPDATE localized_string=VALUES(localized_string), modified=VALUES(created);");
>+        }
>+        // link comments field to translations
>+        $res = $db->execute("UPDATE addons_collections SET comments = {$id} "
>+            ."WHERE collection_id = {$collection_id} AND addon_id = {$addon_id}");
>+        if (false !== $res) {
>+            $this->commit();
>+            return true;
>+        } else {
>+            $this->rollback();
>+            return false;
>+        }
>     }

That felt overly difficult.

>+
>+    /**
>+     * is an add-on part of a given collection?
>+     *
>+     * @param int addon_id
>+     * @param int collection_id
>+     * @return bool true if addon is in collection, false otherwise
>+     */
>+    function isAddonInCollection($addon_id, $collection_id) {
>+        if (!is_numeric($collection_id) || !is_numeric($addon_id)) return null;

I wish we used exceptions.

>@@ -183,10 +195,42 @@ class Collection extends AppModel
>      * @param int $role - role type
>      */
>     function addUser($collectionId, $userId, $role) {
>+        $db =& ConnectionManager::getDataSource($this->useDbConfig);
>+
>+        $collectionId = $db->value($collectionId);
>+        $userId = $db->value($userId);
>+        $role = $db->value($role);
>+
>         return $this->execute("INSERT INTO collections_users (collection_id, user_id, role) VALUES ({$collectionId}, {$userId}, {$role})");

Is there a collections_users?  No?  Damn.

>     }
>     
>     /**
>+     * Remove a user
>+     *
>+     * @param int $collectionId - id of the collection
>+     * @param int $userId - id of the user
>+     */
>+    function removeUser($collectionId, $userId) {
>+        $db =& ConnectionManager::getDataSource($this->useDbConfig);
>+
>+        $collectionId = $db->value($collectionId);
>+        $userId = $db->value($userId);
>+
>+        return $this->execute("DELETE FROM collections_users WHERE user_id = {$userId} AND collection_id={$collectionId}");
>+    }
>+
>+    /**
>+     * Remove all user rights from a collection, by role
>+     * Warning: do not do this with OWNER unless you know what you are doing.
>+     *
>+     * @param int $collection_id
>+     * @param int $role user role to remove, for example COLLECTION_ROLE_ADMIN
>+     */
>+    function removeAllUsersByRole($collection_id, $role) {
>+        return $this->execute("DELETE FROM collections_users WHERE collection_id={$collection_id} AND role={$role};");
>+    }

That sounds scary.  Should you check that it's not ROLE_OWNER?

>diff --git a/site/app/webroot/js/addons.js b/site/app/webroot/js/addons.js
>index 01ef7bb..e01d583 100644
>--- a/site/app/webroot/js/addons.js
>+++ b/site/app/webroot/js/addons.js
>@@ -1290,7 +1290,6 @@ function installButtonAttachInstallMethod(button) {
>     });
> }
> 
>-
> /**
>  * bandwagon: fire a custom refresh event for bandwagon extension
>  * @return void
>@@ -1302,3 +1301,369 @@ function bandwagonRefreshEvent() {
>     bandwagonSubscriptionsRefreshEvent.initEvent("bandwagonRefresh", true, false);
>     element.dispatchEvent(bandwagonSubscriptionsRefreshEvent);
> }
>+
>+/** Collections edit page **/
>+var collections_edit = {
>+    /**
>+     * initialize collections edit page
>+     */
>+    init: function() {
>+        $("#coll-edit > ul").tabs();
>+        $('#coll-edit .jsonly').show();
>+
>+        this.nickname_init();
>+        this.icon_init();
>+        this.user_init();
>+        this.addon_init();
>+        this.addon_comment_init();
>+
>+        $('#delete-coll').click(function(){$(this).hide();$('#delete-confirm').show();}); // delete button

Do we need to hand-compress our javascript now?

>+    },
>+
>+    /**
>+     * initialize nickname check UI
>+     */
>+    nickname_init: function() {
>+        $('#nick-avail').click(this.nickname_check);
>+        $('#CollectionNickname')
>+            .blur(this.nickname_check)
>+            .keypress(function(e){
>+                if (e.which == 13) {

What is 13?  Can we put that in a constant?

>+                    collections_edit.nickname_check();
>+                    return false;
>+                }
>+                return true;
>+            })
>+            .keyup(function(e){if (e.which!=13) collections_edit.nickname_showButton()});
>+    },
>+    /**
>+     * check if a nickname is already taken
>+     */
>+    nickname_check: function() {
>+        var name = $('#CollectionNickname').val();
>+        name = $.trim(name);
>+        $('#CollectionNickname').val(name);

I guess you can't call trim on the val()?  That feels like it should be a one-liner.

Comment 52

[Les Orchard [:lorchard]]

Comment on attachment 376303 [details] [diff] [review]
Fixing the icon test

Looks okay to me

Comment 53

[Jeff Balogh (:jbalogh)]

re: attachment 376187 [details] [diff] [review] 
any logged-in user can get to the edit page of a collection (including those they don't own).  The only thing visible is the add-ons tab, but you can't actually save anything.

Comment 54

[Fred Wenzel [:wenzel]]

(In reply to comment #52)
> (From update of attachment 376303 [details] [diff] [review])
> Looks okay to me

I've committed this to r25381.

Comment 55

[Fred Wenzel [:wenzel]]

Attachment: validate translation fields

This little patch, when called, allows to check fields from a translation box to be validated. It will use your code from bug 491095 -- or any other type of validation.

Comment 56

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 3

(In reply to comment #51)
> (From update of attachment 376187 [details] [diff] [review])
> The good news: it all seems to work.
> The bad news: I complain a lot.  Sorry about the length!
> 
> Usage notes, not looking at the code:
> 
> The whole flash() thing annoys the pants off of me.
> 
> When I click "Update Collection" I expect to go to the /edit page I was on, not
> back to /view.  Is it just me?

Done: I replaced that with a little "success" box on top of the edit page.

> The tabs aren't url-addressable.  There should be something like
> /edit/uuid#permissions to put me on the correct tab.

Yes, they are, try again.

> When I add something from /edit#Add-ons, it flashes "Error saving add-on!" next
> to "Add to Collection".  That doesn't seem right.

I got "Add-on already exists!" -- the event was fired twice, I fixed that.

> "Note: Comment will appear as though written by original publisher on the
> original publication date": I'm not sure what that means.

Fligtar said he'll re-inspect the string wording before it's sent off to the localizers. As the text implies, a comment will show up on the page as if it was written by the publisher of this add-on, at the time this person added the add-on to the collection.

> The collection view isn't updated after I press "Update Collection".  Looks
> like a caching issue since the data is correct from the edit page. -- hmm,
> still incorrect after flipping debug, which usually clears that stuff.  -- the
> name updates, but the list of addons and the icon don't -- maybe it's a problem
> with the listing code?

Sorry, I cannot confirm this. Adding Add-ons takes effect instantly (via AJAX) without even pressing the "update" button.

> The limits on name and description are not enforced.  I have a patch for this
> on bug 491095, but I don't think it will work correctly with the translation
> shenanigans.

I agree, it does not enforce it properly. The patch before contains the framework for this, the new patch will use it.

> I didn't press "Add" on the Permissions page, so the user I entered wasn't
> saved.  I didn't know I had to press Add.  Also, hitting enter in that add-user
> box submits the form without triggering the Add.

Both fixed: you can press enter or click "add" now.

> Check Availability should have a spinner

done.

> I checked the Delete box, forgot about it, made other updates to the
> collection, and then accidentally deleted my treasured collection.  I was sad.

That's why you have to really really have to confirm it twice... If you have a suggestion for improvement, let me know though. I am not a fan of asking 12 times if people really want what they enter though :-/

> "Add to Collection" shows "<addon name> [<addon id>]" right before adding my
> add-on to "Current Add-ons".  It looks weird.

With the change for avoiding the double event, it'll show this now, and allow you to click "add" then.

> I can't undo a Remove from Current Add-ons.

Not sure if we need this in the first iteration. You can just re-add the add-on.

> I have to press Save Comment to get my publisher comments saved.  Then I have
> to press Update Collection to really save it.

No, just "Save Comment" will do.

> The collection nickname is missing when I go to the edit page, and I can't
> enter in the same nickname again.  So once you have a nickname, you can't edit
> again without losing your nickname.

Very good catch, thank you. I fixed this.

> The page looks pretty messed up without js.

It'll be skinned later. Also, non-JS is a graceful fallback only.

> There's 36 validation errors.  I think a lot of those come from the translation box, but not all.

Thanks, I reduced these to 18. Validation bugs in the translation box are definitely another bug, and due to a tag imbalance in there, it causes further validation errors on the page. When these are fixed, we can probably remove the rest on this page. One of the minor ones is an empty <ul></ul> for example, which does not cause harm but won't validate unless it contains <li>s.

> I didn't look at the javascript too much, I was tired.
> 
> > 
> >     function beforeFilter() {
> >-        // Disable ACLs because this controller is entirely public.
> >-        $this->SimpleAuth->enabled = false;
> >-        $this->SimpleAcl->enabled = false;
> 
> This makes it hard to get to the /collections page.

Thanks, I re-added these.

> >@@ -205,51 +197,200 @@ class CollectionsController extends AppController
> >     /**
> >      * Edit collection
> >      */
> >-    function edit($uuid) {
> >+    function edit($uuid = null) {
> 
> Can this function be broken up?  It's 190 lines right now.

Sure. I cut it into three pieces: one general part, one save part, one "get data for view" part.

> >+        if (empty($uuid)) {
> >+            $this->flash(sprintf(_('error_missing_argument'), 'collection_id'), '/', 3);
> >+            return;
> >+        }
> 
> There isn't something lower-level to check this for us?  cake++

Nope, not that I know of.

> >+        if (!empty($this->data)) {
> 
> Should we check validates()?

save() does that.

> >+                // save noscript data
> >+                if ($atleast_manager) {
> >+                    // publishers
> >+                    (...)
> >+  
> >+                    // managers
> >+                    (...)
> 
> Are the publisher and manager blocks above the same?  They look identical, with
> s/Publishers/Managers/g and s/PUBLISHER/ADMIN/g.  Can that handling be moved to
> a helper function?

They are pretty much identical. I put that into another function.

> >+                // add new add-ons
> >+                if (!empty($this->params['form']['q'])) {
> >+                    $_aids = explode(',', $this->params['form']['q']);
> >+                    foreach ($_aids as &$_aid) {
> >+                        $_aid = trim($_aid); if (empty($_aid)) continue;
> 
> Could you do 
> if (!empty($_aid))  {
>   ...addAddonTo...
> }
> 
> It reads better than continue;

Sure.

> >+        // addons
> >+        $addons = $this->AddonCollection->getAddonsFromCollection($id);
> >+        $this->publish('addons', $addons);
> >+        $addons_noscript = array();
> >+        foreach ($addons as &$addon) {
> >+            $addons_noscript[$addon['AddonCollection']['addon_id']] = $addon['Addon']['Translation']['name']['string'];
> >+          }
> >+        $this->publish('addons_noscript', $addons_noscript);
> 
> Does the translationbox handle <noscript> well?

It does not choke horribly, but a short test showed me that you can't edit more than the current locale without JavaScript. That's graceful, but improvable. It's a different bug, though.

> >+    function _checkNickname() {
> 
> Do you want to checkLoggedIn here?

I added it, because it makes sense I guess. Since it's a read-only operation though, I left it out before.

> >+        case 'add':
> >+            if (empty($_POST['email'])) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'email'));
> >+            $roles = array('publishers' => COLLECTION_ROLE_PUBLISHER, 'managers' => COLLECTION_ROLE_ADMIN);
> >+            if (!isset($_POST['role']) || !isset($roles[$_POST['role']]))
> >+                return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'role'));
> >+
> >+            $newuser = $this->User->findByEmail($_POST['email'], array('id'));
> 
> Is it safe to be using $_POST?

No less than any other user data, I assume. However, I replaced it with the more cakey this->params['form']['...'] now.

> >+        $addon_id = $_POST['addon_id'];
> >+        $collection_id = $_POST['collection_id'];
> >+
> >+        $user = $this->Session->read('User');
> >+        $writable = $this->Collection->isWritableByUser($collection_id, $user['id']);
> >+        $role = $this->Collection->getUserRole($collection_id, $user['id']);
> >+        $isadmin = $this->SimpleAcl->actionAllowed('Admin', 'EditAnyCollection', $user);
> >+        $atleast_manager = ($isadmin || in_array($role, array(COLLECTION_ROLE_ADMIN, COLLECTION_ROLE_OWNER)));
> 
> Can this role stuff be moved to a function?  I think I've seen it a few times
> already.

Sure. Done.

> >+        switch ($action) {
> >+        case 'add':
> >+            if ($this->AddonCollection->isAddonInCollection($addon_id, $collection_id))
> >+                return $this->Error->getJSONforError('Add-on already exists!');
> 
> __()?
> ___()?
> ___()?

done.

> >+    function getAddonsFromCollection($collection_id) {
> >+        $db =& ConnectionManager::getDataSource($this->useDbConfig);
> >+        $collection_id = $db->value($collection_id);
> 
> Why do you do this?  I haven't seen that before.

It escapes and quotes values for use in DB queries properly. It's like Amo->clean() minus the pain. Something like this should be used in the future for DB queries, rather than SQL-escaping in the controller (bug 373767).

> >+        $this->unbindFully();
> >+        $res = $this->findAll(array('collection_id' => $collection_id));
> >+        if (!empty($res)) {
> >+            loadModel('Addon');
> >+            loadModel('User');
> >+            if (empty($this->Addon)) $this->Addon =& new Addon();
> >+            if (empty($this->User)) $this->User =& new User();
> 
> These aren't loaded already?

In the model? No.

> >+            foreach ($res as &$row) {
> >+                $row['Addon'] = $this->Addon->getAddon($row['AddonCollection']['addon_id']);
> >+                $user = $this->User->findById($row['AddonCollection']['user_id']);
> >+                $row['User'] = $user['User'];
> 
> clouserw gave me a talkin' to about queries in loops, so <that's going to be
> trouble>.mp3

uhm, okay.

> >+        if (!empty($row[0]['addons_collections']['comments'])) {
> >+            $id = $row[0]['addons_collections']['comments'];
> >+        } else {
> >+            // generate a new primary key id
> >+            $db->execute('UPDATE translations_seq SET id=LAST_INSERT_ID(id+1);');
> >+            $_res = $db->execute('SELECT LAST_INSERT_ID() AS id FROM translations_seq;');
> >+            if ($_row = $db->fetchRow()) {
> >+                $id = $_row[0]['id'];
> >+            } else {
> >+                $this->rollback();
> >+                return false;
> >+            }
> >+        }
> >+        // delete all existing comments
> >+        $db->execute("UPDATE translations SET localized_string = NULL WHERE id = {$id}");
> 
> This piece feels like it should be in a model somewhere.  I want to reuse it.

It was actually partly borrowed from the transaction save code, so it's already there, and it's transparent and all. If the addons_collections table had a single "id" column, we could use saveTranslations(). Since it does not, we have to do this by hand :(

> >+        // link comments field to translations
> >+        $res = $db->execute("UPDATE addons_collections SET comments = {$id} "
> >+            ."WHERE collection_id = {$collection_id} AND addon_id = {$addon_id}");
> >+        if (false !== $res) {
> >+            $this->commit();
> >+            return true;
> >+        } else {
> >+            $this->rollback();
> >+            return false;
> >+        }
> >     }
> 
> That felt overly difficult.

Yes. Though I thought it to be mildly better than adding an "id" column to the addons_collections model. If you disagree, please let me know and I'll amend the DB schema.

> >+    function isAddonInCollection($addon_id, $collection_id) {
> >+        if (!is_numeric($collection_id) || !is_numeric($addon_id)) return null;
> 
> I wish we used exceptions.

now that we've been on PHP5 for awhile, that's an option.

> >     function addUser($collectionId, $userId, $role) {
> >+        $db =& ConnectionManager::getDataSource($this->useDbConfig);
> >+
> >+        $collectionId = $db->value($collectionId);
> >+        $userId = $db->value($userId);
> >+        $role = $db->value($role);
> >+
> >         return $this->execute("INSERT INTO collections_users (collection_id, user_id, role) VALUES ({$collectionId}, {$userId}, {$role})");
> 
> Is there a collections_users?  No?  Damn.

A model? No. In this case, it would not add much besides Cake overhead.

> >+    /**
> >+     * Remove all user rights from a collection, by role
> >+     * Warning: do not do this with OWNER unless you know what you are doing.
> >+     *
> >+     * @param int $collection_id
> >+     * @param int $role user role to remove, for example COLLECTION_ROLE_ADMIN
> >+     */
> >+    function removeAllUsersByRole($collection_id, $role) {
> >+        return $this->execute("DELETE FROM collections_users WHERE collection_id={$collection_id} AND role={$role};");
> >+    }
> 
> That sounds scary.  Should you check that it's not ROLE_OWNER?

I don't think it's the model's job to check access rights. Removing the owner (for example by a site admin) would orphan a collection and make it deletable by site admins only. So far, the only right that an owner has on top of what a collection admin can do is deleting the collection.

> >+        $('#delete-coll').click(function(){$(this).hide();$('#delete-confirm').show();}); // delete button
> 
> Do we need to hand-compress our javascript now?

What does that mean? I put this into several lines. I kept the fadeOut() commands in single lines, as it's a single logical operation.

> >+    nickname_init: function() {
> >+        $('#nick-avail').click(this.nickname_check);
> >+        $('#CollectionNickname')
> >+            .blur(this.nickname_check)
> >+            .keypress(function(e){
> >+                if (e.which == 13) {
> 
> What is 13?  Can we put that in a constant?

13 is the ASCII code for enter. I put it in a constant.

> >+    nickname_check: function() {
> >+        var name = $('#CollectionNickname').val();
> >+        name = $.trim(name);
> >+        $('#CollectionNickname').val(name);
> 
> I guess you can't call trim on the val()?  That feels like it should be a
> one-liner.

Makes sense. Done.




... your comments took me awhile to go through but I am thankful for the thorough code review you gave! I feel like it made this patch a lot better.

Comment 57

[Fred Wenzel [:wenzel]]

(In reply to comment #53)
> re: attachment 376187 [details] [diff] [review] 
> any logged-in user can get to the edit page of a collection (including those
> they don't own).  The only thing visible is the add-ons tab, but you can't
> actually save anything.

I forgot about this, will need to give you a new patch. Apparently there's a bug in the Collection model's "isWritableByUser" method. If it were working as expected, the page would not be shown.

Comment 58

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 3, amended

Including the fix for the isWritable... method.

Comment 59

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 3, amended

Uhm, patch-making fail.

Comment 60

[Jeff Balogh (:jbalogh)]

Comment on attachment 376516 [details] [diff] [review]
validate translation fields

This would be extra-awesome if it had tests.

Comment 61

[Jeff Balogh (:jbalogh)]

Comment on attachment 376805 [details] [diff] [review]
Patch, rev. 3, amended

BUG: can't set a collection nickname: Notice: Undefined offset: 0 in /home/jbalogh/public_html/amo/site/app/models/collection.php on line 473 

RFE: can't upgrade from publisher to manager with deleting from publisher first

RFE: re: delete, put a red bar at the top, change 'update collection' to 'delete collection'

RFE: changing tabs should update the URL

RFE: some things are ajax, some things are not is a bit confusing from the user's perspective

RFE: spinner for adding users

RFE: undo for removing add-ons

> if (!$rights['writable'] && !$rights['isadmin']) {

DeMorgan'ing that into !(writable || isadmin) reads better to me, and I wanted to impress you with my CS math knowledge. ;)  Whatever.

> $this->publish('collection_url', '/collection/'.(empty($updated['Collection']['nickname']) ? $updated['Collection']['uuid'] : $updated['Collection']['nickname']), true);

I would put $nickname in a variable since my religion enforces strict line-length requirements.  And I find it easier to read.

> if (empty($action)) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'action'));

Again, since my religion requires newlines after ifs, I object to the return on the same line for readability.

> + foreach ($res as &$row) {
> +     $row['Addon'] = $this->Addon->getAddon($row['AddonCollection']['addon_id']);
> +     $user = $this->User->findById($row['AddonCollection']['user_id']);
> +     $row['User'] = $user['User'];
> + }

Is it possible to grab all this with joins instead of a loop?

> +var KEYCODE_ENTER = 13;

Nice!

> +String.prototype.nl2br = function() {

Why put this on the prototype instead of making the function available somewhere?  Is this going to break an for each loops?

> +    return this.replace(/([^>]?)\n/g, '$1<br/>');

The piece of the regex in parens says "match 0 or 1 characters that aren't a close bracket".  That matches "<>\n" since 0 characters always matches.  What's the intent of the negative match?  Same question on br2nl.

> $('#saved_success').animate({opacity: 1.0}, 10000,function(){ $(this).fadeOut('normal',function(){$(this).remove()}); });

As you can imagine, my religion has things to say about function indentation as well.  That's a lot to stuff in one line.

> + .keypress(function(e){
> +     if (e.which == KEYCODE_ENTER) {
> +         collections_edit.nickname_check();
> +         return false;
> +     }
> +     return true;
> + })

Is it better to use preventDefault or something like that?

> + if ($('#CollectionNickname').val().length > 0) {
> +     $('#nick-avail').show();
> + } else {
> +     $('#nick-avail').hide();
> + }

$('#nick-avail')[$('#CollectionNickname').val().length > 0 ? 'show' : 'hide']();

Sometimes I like to code-golf too.  My way is better for job security, but that's about the only place it wins. :)

Comment 62

[Fred Wenzel [:wenzel]]

(In reply to comment #61)
> (From update of attachment 376805 [details] [diff] [review])
> BUG: can't set a collection nickname: Notice: Undefined offset: 0 in
> /home/jbalogh/public_html/amo/site/app/models/collection.php on line 473 

Fixed.

> RFE: can't upgrade from publisher to manager with deleting from publisher first

I'll leave this one for a later date. In particular, I am unsure if it is a strong UX improvement to just do the "promotion" or "demotion" automatically, with the user possibly being unaware of changing an existing user.

> RFE: re: delete, put a red bar at the top, change 'update collection' to
> 'delete collection'

good suggestion. done!

> RFE: changing tabs should update the URL

done. QA will need to make sure this works with IE as expected, though!

> RFE: some things are ajax, some things are not is a bit confusing from the
> user's perspective

We can smooth that out by making more things ajaxy in the future.

> RFE: spinner for adding users

Done.

> RFE: undo for removing add-ons

something for the future as well.

> > if (!$rights['writable'] && !$rights['isadmin']) {
> 
> DeMorgan'ing that into !(writable || isadmin) reads better to me, and I wanted
> to impress you with my CS math knowledge. ;)  Whatever.

np.

> > $this->publish('collection_url', '/collection/'.(empty($updated['Collection']['nickname']) ? $updated['Collection']['uuid'] : $updated['Collection']['nickname']), true);
> 
> I would put $nickname in a variable since my religion enforces strict
> line-length requirements.  And I find it easier to read.

I split it up into a more readable if statement.

> > if (empty($action)) return $this->Error->getJSONforError(sprintf(_('error_missing_argument'), 'action'));
> 
> Again, since my religion requires newlines after ifs, I object to the return on
> the same line for readability.

Hm, for single-line if blocks, that works.

> > + foreach ($res as &$row) {
> > +     $row['Addon'] = $this->Addon->getAddon($row['AddonCollection']['addon_id']);
> > +     $user = $this->User->findById($row['AddonCollection']['user_id']);
> > +     $row['User'] = $user['User'];
> > + }
> 
> Is it possible to grab all this with joins instead of a loop?

Our automatic dynamic translation fetching won't work on secondary models that Cake pulls in, so no, not easily.

> > +var KEYCODE_ENTER = 13;
> 
> Nice!
> 
> > +String.prototype.nl2br = function() {
> 
> Why put this on the prototype instead of making the function available
> somewhere?  Is this going to break an for each loops?

I made a jQuery utility plugin out of it.

> > +    return this.replace(/([^>]?)\n/g, '$1<br/>');
> 
> The piece of the regex in parens says "match 0 or 1 characters that aren't a
> close bracket".  That matches "<>\n" since 0 characters always matches.  What's
> the intent of the negative match?  Same question on br2nl.

That's what I get for peeking into other people's code and not thinking ;) I replaced it with the obvious.

> > $('#saved_success').animate({opacity: 1.0}, 10000,function(){ $(this).fadeOut('normal',function(){$(this).remove()}); });
> 
> As you can imagine, my religion has things to say about function indentation as
> well.  That's a lot to stuff in one line.

As mentioned, this is a single logical operation: "after 10 seconds, fade out and remove this element". Admittedly, I should probably drop this into a jQuery plugin.

Done. Actually, two plugins: delay() and fadeRemove().

> > + .keypress(function(e){
> > +     if (e.which == KEYCODE_ENTER) {
> > +         collections_edit.nickname_check();
> > +         return false;
> > +     }
> > +     return true;
> > + })
> 
> Is it better to use preventDefault or something like that?

done.

> > + if ($('#CollectionNickname').val().length > 0) {
> > +     $('#nick-avail').show();
> > + } else {
> > +     $('#nick-avail').hide();
> > + }
> 
> $('#nick-avail')[$('#CollectionNickname').val().length > 0 ? 'show' :
> 'hide']();
> 
> Sometimes I like to code-golf too.  My way is better for job security, but
> that's about the only place it wins. :)

Yeah, not gonna happen ;) The newer version of jQuery actually allows the syntax show(boolean) to show/hide something conditionally. Ours does not have that yet though, and upgrading jQuery is out of scope here.

Comment 63

[Fred Wenzel [:wenzel]]

Committed to r25582! QA: Please feel free to go ahead and QA the collections edit page. Note that the design of the page will be improved with bug 490897.

Comment 64

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on https://preview.addons.mozilla.org/en-US/firefox/collections; it's been implemented -- Krupa's been filing separate 5.0.6-targeted bugs (won't mention them here) as she finds them, and I'll be testing this too.

Comment 65

[Fred Wenzel [:wenzel]]

Comment on attachment 376134 [details]
Incremental SQL

Marking this SQL obsolete, because I will need to change the "comments" foreign key over in bug 493117.

Comment 66

[Fred Wenzel [:wenzel]]

Comment on attachment 376134 [details]
Incremental SQL

actually, I don't (sigh). But I'll add this to the DB page.

Comment 67

[Stephen Donner [:stephend] Not actively reading bugmail]

removing "push-needed" from 105 AMO 5.0.6 bugs; filter on "I hate stephend!"