Closed Bug 456132 Opened 14 years ago Closed 14 years ago

[W-1.4] Collection Management

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: fligtar, Assigned: wenzel)

References

()

Details

(Whiteboard: [not blocked by clearleft])

Attachments

(9 files, 11 obsolete files)

410.81 KB, image/png
Details
3.29 KB, patch
jbalogh
: review+
Details | Diff | Splinter Review
723 bytes, text/plain
jbalogh
: review+
Details
2.17 KB, patch
jbalogh
: review+
Details | Diff | Splinter Review
2.77 KB, patch
jbalogh
: review+
Details | Diff | Splinter Review
3.07 KB, patch
jbalogh
: review+
Details | Diff | Splinter Review
1.48 KB, patch
lorchard
: review+
Details | Diff | Splinter Review
946 bytes, patch
jbalogh
: review+
Details | Diff | Splinter Review
86.26 KB, patch
jbalogh
: review+
Details | Diff | Splinter Review
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.
Priority: -- → P2
Target Milestone: --- → 4.0.5
Assignee: nobody → jboriss
Whiteboard: [mocks by 12/15]
Attached image draft 1: feeds view (obsolete) —
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.
Attached image draft 1: password protect (obsolete) —
User protects his feed with a password
Attached image draft 1: restrict to email addresses (obsolete) —
User makes his feed only able to certain email addresses (where email addresses could be bandwagon logins, AMO logins, etc)
Attached image draft 1: deletiong a feed (obsolete) —
User deletes a feed
Target Milestone: 4.0.5 → 5.0.1
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?)?
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.
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.
(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.
Target Milestone: 5.0.1 → 5.0.2
Target Milestone: 5.0.2 → ---
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
Priority: P2 → --
Summary: Collections Management → [W-1.4] [Design] Collection Management
Whiteboard: [mocks by 12/15]
Target Milestone: --- → BW-M2
Version: 3.2 → unspecified
->M3.
Target Milestone: BW-M2 → BW-M3
Assignee: jboriss → nobody
Summary: [W-1.4] [Design] Collection Management → [W-1.4] Collection Management
Target Milestone: BW-M3 → BW-M4
Attached image updated management wireframe, v1 (obsolete) —
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.
Attachment #353042 - Attachment is obsolete: true
Attachment #353043 - Attachment is obsolete: true
Attachment #353044 - Attachment is obsolete: true
Attachment #353045 - Attachment is obsolete: true
Associated application Advanced - shouldn't that be a multi-select?
I like it
(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.
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.
wireframe looks fine to me- wondering if "add-ons" should be the second tab but I don't feel strongly about it.
Assignee: nobody → fwenzel
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.
Depends on: 488154
Depends on: 484878
Depends on: 488487
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?
Moving to revised M5 milestone (4/28).
Target Milestone: BW-M4 → BW-M5
Good point. How about this for JS-enabled and JS-disabled?
Attachment #370544 - Attachment is obsolete: true
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.
Attached file SQL for collection management (obsolete) —
Here it comes. First, some SQL.
Attached patch Patch, rev. 1 (obsolete) — Splinter Review
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)?
Attachment #374320 - Flags: review?(fligtar)
Status: NEW → ASSIGNED
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 on attachment 374320 [details] [diff] [review]
Patch, rev. 1

Sure, no prob.
Attachment #374320 - Flags: review?(fligtar) → review?(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.
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.
Ryan, can you review this soon?
Target Milestone: BW-M5 → 5.0.6
Blocks: 490897
Whiteboard: [not blocked by clearleft]
Attached patch Patch, rev. 2 (obsolete) — Splinter Review
Attachment #375720 - Flags: review?
Attachment #375720 - Flags: review? → review?(rdoherty)
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.
Attachment #374320 - Attachment is obsolete: true
Attachment #374320 - Flags: review?(rdoherty)
Blocks: 456139
Blocks: 491762
Attachment #375720 - Flags: review?(clouserw)
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 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.
Attachment #375720 - Attachment is obsolete: true
Attachment #375720 - Flags: review?(rdoherty)
Attachment #375720 - Flags: review?(clouserw)
Attachment #374310 - Attachment is obsolete: true
Attached patch Helper componentSplinter Review
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.
Attachment #376126 - Flags: review?(jbalogh)
Duplicate of this bug: 491762
Attached file Incremental SQL
Here's the incremental SQL, incorporating the changes from bug 491762 as well.
Attachment #376134 - Flags: review?(jbalogh)
These are the same SQL changes, in the SQL schema file.
Attachment #376135 - Flags: review?(jbalogh)
Collection deletion is now done as part of the edit page, so we can remove its scaffolding altogether.
Attachment #376150 - Flags: review?(jbalogh)
This patch is straightforward: it is the code for displaying a collection icon. It basically mirrors what we have for addon icons.
Attachment #376157 - Flags: review?(jbalogh)
Attached patch Actual collections edit patch (obsolete) — Splinter Review
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.
Attachment #376187 - Flags: review?(jbalogh)
Attachment #376126 - Flags: review?(jbalogh) → review+
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 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.
Attachment #376134 - Flags: review?(jbalogh) → review-
Attachment #376135 - Flags: review?(jbalogh) → review-
Comment on attachment 376135 [details] [diff] [review]
SQL changes to repository

Parts of this already exist in trunk.
Comment on attachment 376150 [details] [diff] [review]
Remove delete scaffolding

I guess this is moving to edit in a future patch?  Deleting_code++
Attachment #376150 - Flags: review?(jbalogh) → review+
Comment on attachment 376135 [details] [diff] [review]
SQL changes to repository

Woops, I was looking at a dirty working tree.  This does work.
Attachment #376135 - Flags: review- → review+
Attachment #376134 - Flags: review- → review+
Comment on attachment 376134 [details]
Incremental SQL

Ditto.  Sorry 'bout that.
(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.
The SQL changes are committed to r25340, and the final days of the "delete" scaffolding are r25341.
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. :)
Attachment #376157 - Flags: review?(jbalogh) → review+
(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.
I made the sharing API's icon URL absolute, then fixed the respective test.
Attachment #376303 - Flags: review?(lorchard)
Attachment #376187 - Flags: review?(jbalogh) → review-
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 on attachment 376303 [details] [diff] [review]
Fixing the icon test

Looks okay to me
Attachment #376303 - Flags: review?(lorchard) → review+
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.
(In reply to comment #52)
> (From update of attachment 376303 [details] [diff] [review])
> Looks okay to me

I've committed this to r25381.
Blocks: 492115
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.
Attachment #376516 - Flags: review?(jbalogh)
Attached patch Patch, rev. 3 (obsolete) — Splinter Review
(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.
Attachment #376187 - Attachment is obsolete: true
Attachment #376798 - Flags: review?(jbalogh)
(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.
Attached patch Patch, rev. 3, amended (obsolete) — Splinter Review
Including the fix for the isWritable... method.
Attachment #376798 - Attachment is obsolete: true
Attachment #376804 - Flags: review?(jbalogh)
Attachment #376798 - Flags: review?(jbalogh)
Uhm, patch-making fail.
Attachment #376804 - Attachment is obsolete: true
Attachment #376805 - Flags: review?(jbalogh)
Attachment #376804 - Flags: review?(jbalogh)
Comment on attachment 376516 [details] [diff] [review]
validate translation fields

This would be extra-awesome if it had tests.
Attachment #376516 - Flags: review?(jbalogh) → review+
Blocks: 492447
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. :)
Attachment #376805 - Flags: review?(jbalogh) → review+
Blocks: 492461
Blocks: 492462
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: push-needed
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
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.
Attachment #376134 - Attachment is obsolete: true
Comment on attachment 376134 [details]
Incremental SQL

actually, I don't (sigh). But I'll add this to the DB page.
Attachment #376134 - Attachment is obsolete: false
removing "push-needed" from 105 AMO 5.0.6 bugs; filter on "I hate stephend!"
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.