Closed
Bug 492653
Opened 16 years ago
Closed 16 years ago
Subscribe/unsubscribe pages
Categories
(addons.mozilla.org Graveyard :: Collections, defect)
addons.mozilla.org Graveyard
Collections
Tracking
(Not tracked)
VERIFIED
FIXED
5.0.6
People
(Reporter: wenzel, Assigned: wenzel)
References
()
Details
Attachments
(4 files, 2 obsolete files)
266 bytes,
text/html
|
Details | |
11.97 KB,
patch
|
wenzel
:
review+
|
Details | Diff | Splinter Review |
3.53 KB,
patch
|
jbalogh
:
review+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
wenzel
:
review+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
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.
Assignee: fligtar → nobody
Updated•16 years ago
|
Assignee: nobody → fwenzel
Assignee | ||
Comment 2•16 years ago
|
||
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?
Blocks: 456131
Comment 3•16 years ago
|
||
wenzel: I haven't started working on this yet. I forgot about the lightbox with subscription stuff until I was uploading my patch.
Assignee | ||
Comment 4•16 years ago
|
||
This is the first iteration of the new subscription pages. I combined the two in one view.
Attachment #378574 -
Flags: review?(jbalogh)
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment 5•16 years ago
|
||
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.
Attachment #378574 -
Flags: review?(jbalogh) → review+
Assignee | ||
Comment 6•16 years ago
|
||
(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•16 years ago
|
||
(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. :)
Assignee | ||
Comment 8•16 years ago
|
||
(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.
Assignee | ||
Comment 9•16 years ago
|
||
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.
Attachment #379095 -
Flags: review?(jbalogh)
Assignee | ||
Comment 10•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Comment 12•16 years ago
|
||
Updated version of patch 1 (already r+ed)
Attachment #378574 -
Attachment is obsolete: true
Attachment #379294 -
Flags: review+
Assignee | ||
Comment 13•16 years ago
|
||
This is the updated patch 2. Our recent check-ins (mine included) did indeed lead to major bit-rot here :-/
Attachment #379095 -
Attachment is obsolete: true
Attachment #379295 -
Flags: review?(jbalogh)
Attachment #379095 -
Flags: review?(jbalogh)
Comment 14•16 years ago
|
||
Attachment #379751 -
Flags: review?
Updated•16 years ago
|
Attachment #379751 -
Flags: review? → review?(fwenzel)
Comment 15•16 years ago
|
||
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.
Attachment #379295 -
Flags: review?(jbalogh) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #379751 -
Flags: review?(fwenzel) → review+
Assignee | ||
Comment 16•16 years ago
|
||
Comment on attachment 379751 [details] [diff] [review]
Get subscriptions working with the new system
That works, thanks for adding these missing bits.
Assignee | ||
Comment 17•16 years ago
|
||
Verified FIXED; ran through add/remove functionality both with and without JavaScript enabled -- looks and works great!
Status: RESOLVED → VERIFIED
removing "push-needed" from 105 AMO 5.0.6 bugs; filter on "I hate stephend!"
Keywords: push-needed
Updated•9 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•