Closed Bug 492653 Opened 16 years ago Closed 16 years ago

Subscribe/unsubscribe pages

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: wenzel, Assigned: wenzel)

References

()

Details

Attachments

(4 files, 2 obsolete files)

These two pages are scaffolding only at the moment. What should the layout and wording be here, where should they point the user?
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
Assignee: nobody → fwenzel
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
Depends on: 493820
wenzel: I haven't started working on this yet. I forgot about the lightbox with subscription stuff until I was uploading my patch.
Blocks: 491755
Attached patch Patch, rev. 1 (obsolete) — Splinter Review
This is the first iteration of the new subscription pages. I combined the two in one view.
Attachment #378574 - Flags: review?(jbalogh)
Status: NEW → ASSIGNED
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+
(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.
(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. :)
(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.
Attached patch Patch, rev. 2 (obsolete) — Splinter Review
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)
Attached file 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.
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.
Updated version of patch 1 (already r+ed)
Attachment #378574 - Attachment is obsolete: true
Attachment #379294 - Flags: review+
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)
Blocks: 494863
Attachment #379751 - Flags: review? → review?(fwenzel)
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+
Attachment #379751 - Flags: review?(fwenzel) → review+
Comment on attachment 379751 [details] [diff] [review] Get subscriptions working with the new system That works, thanks for adding these missing bits.
Committed to r26329, r26330, and r26331.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: push-needed
Resolution: --- → FIXED
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
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: