492653 - Subscribe/unsubscribe pages

Status: VERIFIED FIXED

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

Description

[Fred Wenzel [:wenzel]]

These two pages are scaffolding only at the moment.

What should the layout and wording be here, where should they point the user?

Comment 1

[Justin Scott [:fligtar]]

The subscribe ("add to favorites") page for JS enabled browsers is actually a lightbox with AJAX, as shown here:

https://bug456131.bugzilla.mozilla.org/attachment.cgi?id=375866

If someone has JS disabled, the page should have the same text that is shown in that lightbox, but without the typos. Also, the My Favorites tab should be a link to that tab in the directory page.

For unsubscribe ("remove from favorites") the text can be:

"[collection name] has been removed from your favorite collections." at the top of that collection's display page.

Note that on any pages that subscribe or unsubscribe, AJAX or not, we should trigger the bandwagon event so that the extension will update.

Thanks for catching, Fred.

Comment 2

[Fred Wenzel [:wenzel]]

jbalogh: Were you planning on including that in your patch? Either way, I should probably wait on you here.

I could, however, skin the subscribe/unsubscribe page, then you just have to POST to it, and display the right <div> from the response in a lightbox. (That'd also fix bug 491755).

Not sure what to do about the "don't show this again" yet, though. Yet another new database field?

Comment 3

[Jeff Balogh (:jbalogh)]

wenzel: I haven't started working on this yet.  I forgot about the lightbox with  subscription stuff until I was uploading my patch.

Comment 4

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 1

This is the first iteration of the new subscription pages. I combined the two in one view.

Comment 5

[Jeff Balogh (:jbalogh)]

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

For the most part, this looks good.  Is there a better way to get the subscribe_ajax part than pulling down the whole page and selecting that part?

Should the controller enforce POST?

collections_index_li_favorites isn't in messages.po


>diff --git a/site/app/controllers/collections_controller.php b/site/app/controllers/collections_controller.php
>index ca4bdc2..7cdd33a 100644
>--- a/site/app/controllers/collections_controller.php
>+++ b/site/app/controllers/collections_controller.php
>@@ -216,35 +216,57 @@ class CollectionsController extends AppController

>+    /**
>+     * Combined function for subscribing/unsubscribing. Action is determined by
>+     * $this->action.
>+     * @access private
>+     * @param string $uuid collection UUID or nickname
>+     * @return void
>+     */
>+    function _subscribe_unsubscribe($uuid) {
>         $this->Amo->checkLoggedIn(); // must be logged in
>         $id = $this->Collection->getIdForUuidOrNickname($uuid);
>-        
>-        if(!$id || !is_numeric($id)) {
>-            $this->flash(sprintf(_('error_missing_argument'), 'collection_id'), '/', 3);
>+
>+        if (!in_array($this->action, array('subscribe', 'unsubscribe'))) {
>+            $this->flash(___('error_access_denied'), '/', 3);
>             return;
>         }

Can this be a 400 Bad Request?

>-        
>+
>+        if (!$id || !is_numeric($id)) {
>+            $this->set('success', false);
>+            $this->render('subscribe');
>+            return;
>+        }

Just curious: would it be possible to `return $this->render('subscribe')`.

>+            } elseif ($this->action == 'unsubscribe') {
>+                if (!empty($success)) {
>+                    echo $this->renderElement('notification', array(
>+                        'type' => 'success',
>+                        'msg' => sprintf(___('collections_unsubscribe_success'),
>+                            $collection['Translation']['name']['string'])
>+                    ));

I'm loving this notification element.

Comment 6

[Fred Wenzel [:wenzel]]

(In reply to comment #5)
> (From update of attachment 378574 [details] [diff] [review])
> For the most part, this looks good.  Is there a better way to get the
> subscribe_ajax part than pulling down the whole page and selecting that part?

I'll investigate that.

> Should the controller enforce POST?

Yes, that's on my list next.

> collections_index_li_favorites isn't in messages.po

Right, it's part of your own patch in bug 456131.

> >+        if (!in_array($this->action, array('subscribe', 'unsubscribe'))) {
> >+            $this->flash(___('error_access_denied'), '/', 3);
> >             return;
> >         }
> 
> Can this be a 400 Bad Request?

That's bug 492300.

> >+        if (!$id || !is_numeric($id)) {
> >+            $this->set('success', false);
> >+            $this->render('subscribe');
> >+            return;
> >+        }
> 
> Just curious: would it be possible to `return $this->render('subscribe')`.

Sure. Why would we do this?

> >+            } elseif ($this->action == 'unsubscribe') {
> >+                if (!empty($success)) {
> >+                    echo $this->renderElement('notification', array(
> >+                        'type' => 'success',
> >+                        'msg' => sprintf(___('collections_unsubscribe_success'),
> >+                            $collection['Translation']['name']['string'])
> >+                    ));
> 
> I'm loving this notification element.

I do, too :) On a side note, morgamic suggested overriding flash() with a page containing this. That'd be instant prettiness for the release.

Comment 7

[Jeff Balogh (:jbalogh)]

(In reply to comment #6)
> > collections_index_li_favorites isn't in messages.po
> 
> Right, it's part of your own patch in bug 456131.

You expect me to remember that?

> > >+        if (!$id || !is_numeric($id)) {
> > >+            $this->set('success', false);
> > >+            $this->render('subscribe');
> > >+            return;
> > >+        }
> > 
> > Just curious: would it be possible to `return $this->render('subscribe')`.
> 
> Sure. Why would we do this?

It feels better to me.  `return $this->render` says "render is the last thing I need to do, so return here."  Plus, 1 line is better than 2. :)

Comment 8

[Fred Wenzel [:wenzel]]

(In reply to comment #7)
> > > Just curious: would it be possible to `return $this->render('subscribe')`.
> > 
> > Sure. Why would we do this?
> 
> It feels better to me.  `return $this->render` says "render is the last thing I
> need to do, so return here."  Plus, 1 line is better than 2. :)

I just checked the Cake API and yes, we can do this. It does not return anything ridiculously huge (not an unwarranted fear, for Cake) but instead boolean true if rendering worked, false otherwise.

Comment 9

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 2

This patch is another revision *on top* of the first patch.

It now enforces "uuid" as a POST-only parameter, likewise our session check variable needs to be set correctly.

If you use this as an AJAX action, you should use the URL .../subscribe/ajax instead of just .../subscribe/. That'll result in just the plain HTML of the success/error message being returned, which you should be able to just display in the right spot of your current page then.

Comment 10

[Fred Wenzel [:wenzel]]

Attachment: HTML form for testing

This is a fun little HTML form that you can use to test this code. Just fix the form URL, then put a collection UUID in one field, and the proper session check code in the other.

Comment 11

[Jeff Balogh (:jbalogh)]

These patches need a rebase.  Also, the new collection pages are posting an input named "collection" with the uuid; can you use that?

And I'll probably ask for the part that gets rendered when is_ajax=true to move to an element instead of threading that check throughout the code.

Comment 12

[Fred Wenzel [:wenzel]]

Attachment: Replacing Patch 1

Updated version of patch 1 (already r+ed)

Comment 13

[Fred Wenzel [:wenzel]]

Attachment: Replacing Patch 2

This is the updated patch 2. Our recent check-ins (mine included) did indeed lead to major bit-rot here :-/

Comment 14

[Jeff Balogh (:jbalogh)]

Attachment: Get subscriptions working with the new system

Comment 15

[Jeff Balogh (:jbalogh)]

Comment on attachment 379295 [details] [diff] [review]
Replacing Patch 2

The collections_success_delete string is a duplicate, and you need the patch I uploaded to keep everything working.  Other than that, it looks good.

Comment 16

[Fred Wenzel [:wenzel]]

Comment on attachment 379751 [details] [diff] [review]
Get subscriptions working with the new system

That works, thanks for adding these missing bits.

Comment 17

[Fred Wenzel [:wenzel]]

Committed to r26329, r26330, and r26331.

Comment 18

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

Verified FIXED; ran through add/remove functionality both with and without JavaScript enabled -- looks and works great!

Comment 19

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

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