481149 - [E-1.1] Add-ons Manager Integration Changes

Status: RESOLVED FIXED

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

Description

[Brian King [:kinger]]

Auto-publisher collections are visually distinct from other collections in the Subscriptions pane.

Is this designed yet?

Comment 1

[Justin Scott [:fligtar]]

Morphing this bug to reflect all the UI updates to the existing Add-ons Manager Integration.

Comment 2

[Justin Scott [:fligtar]]

Attachment: Add-ons Manager Integration mockup, v1

This is a mockup of how the various states of the Add-ons Manager Subscriptions pane should look.

Comment 3

[Justin Scott [:fligtar]]

Attachment: Install Button mockup, v1

This is a mockup of how the extension install button should look.

Comment 4

[Brian King [:kinger]]

Based on Add-ons Manager Integration mockup, v1, here are the changes and missing features I see needed from the current implementation:

1) View Site button becomes a View Collection link
2) Remove button becomes Unsubscribe link
3) Update button needs to be removed
4) In right panel, the header needs to be expanded to include an icon and a description if the subscription
5) Publisher Comments integration
6) Special treatment for auto-publisher subscriptions at the top of the listing in the left panel.

Comment 5

[Brian King [:kinger]]

(In reply to comment #4)
> 1) View Site button becomes a View Collection link
> 2) Remove button becomes Unsubscribe link
> 3) Update button needs to be removed

Done. I've filed bug 481409 to handle state and action if these links.

Comment 6

[Justin Scott [:fligtar]]

Yes, that list of changes looks about right. I would also add that I removed the "Category" from underneath the "Added" part, and instead we display who published the add-on to the collection when expanded.

I'm not sure if we had the "no subscriptions" state coded already, but we now have text and layout for that as well (before it was described as only being in the right panel).

Comment 7

[Brian King [:kinger]]

(In reply to comment #6)
> I'm not sure if we had the "no subscriptions" state coded already, but we now
> have text and layout for that as well (before it was described as only being in
> the right panel).

I broke out bug 481628 for that.

Comment 8

[David McNamara [:mackers]]

A quick review of the elements of this bug.

1) View Site button becomes a View Collection link

The button is there, but the view collection link goes nowhere. I imagine the link should go to http://%AMO_HOST%/%LOCALE%/firefox/collections/view/%COLLECTION_ID%

However, we can't programatically create this URL, as the API instructs us not to create URLs based on a collection id, but instead to follow hrefs as provided by the REST API.

This href is not available in the API.

2) Remove button becomes Unsubscribe link

As above. The button is there, but we can't construct the URL.

3) Update button needs to be removed

Done.

4) In right panel, the header needs to be expanded to include an icon and a
description if the subscription

The description is now there. I don't see an icon url anywhere.

5) Publisher Comments integration

This is done, but is displaying "Unknown" as the comment author due to missing information in the API.

6) Special treatment for auto-publisher subscriptions at the top of the listing in the left panel.

TBD.

Comment 9

[Justin Scott [:fligtar]]

Hi Les, can you take a look at comment #8?

Comment 10

[Les Orchard [:lorchard]]

The IDs are changing to UUIDs, so it still good not to try programmatically creating URLs.  What, exactly, are the missing pieces of information?  Is this the list:

* URL to view collection in browser
* URL to unsubscribe from collection in browser
* Name of person adding an addon to a collection

The third thing is actually supported by the API, but I think the scaffolding isn't adding it.

Anything else?

Comment 11

[Les Orchard [:lorchard]]

Attachment: patch to include user_id when adding addon to collection in scaffolding, as well as additional links in collections in API

Okay, have a patch here that causes the scaffolding to start annotating addons with who added it to a collection, and some tweaks to the API to include view/subscribe/unsubscribe links suitable for visitation in a browser.

Need a reviewer since rdoherty's out, alas.

Also, the XML produced will look like this (a <links> element added to <collection> elements):

<?xml version="1.0" encoding="utf-8" ?>
<sharing xmlns="http://addons.mozilla.org/" xml:base="http://dev-bandwagon.addons.mozilla.org/en-US/firefox/api/1.3/sharing/">
    <email href="email" />
    <collections href="collections/">
        
        <collection 
            href="collections/b0144e6f-b31d-25f0-d91c-b4b356e23940/" 
            name="floof"
            description="flooz"
            creator="Sancus"
            listed="yes" writable="yes" subscribed="yes" 
            lastmodified="2009-03-11T18:15:28-07:00">
                <links xml:base="http://dev-bandwagon.addons.mozilla.org/en-US/firefox/">
                    <link id="view" href="collections/view/b0144e6f-b31d-25f0-d91c-b4b356e23940" />
                    <link id="subscribe" href="collections/subscribe/b0144e6f-b31d-25f0-d91c-b4b356e23940" />
                    <link id="unsubscribe" href="collections/unsubscribe/b0144e6f-b31d-25f0-d91c-b4b356e23940" />
                </links>
                <addons href="collections/b0144e6f-b31d-25f0-d91c-b4b356e23940/addons/" />
        </collection>
        
    </collections>
</sharing>

Comment 12

[Les Orchard [:lorchard]]

Comment on attachment 366961 [details] [diff] [review]
patch to include user_id when adding addon to collection in scaffolding, as well as additional links in collections in API

tossing an r? to clouserw, since rdoherty's out.  hopefully the addons bandwagon branch doesn't give too much trouble

Comment 13

[Ryan Doherty (:rdoherty)]

Comment on attachment 366961 [details] [diff] [review]
patch to include user_id when adding addon to collection in scaffolding, as well as additional links in collections in API

Tested, looks good and works.

Comment 14

[Les Orchard [:lorchard]]

checked the patch in as r23587

Comment 15

[Brian King [:kinger]]

I think this has broken:
https://bandwagon.stage.mozilla.com/en-US/firefox/collections

When trying to subscribe to a collection there, I get:

Missing argument: collection_id

Feed subscriptions are behaving oddly in the extension as well, but that is probably just our code needing to play catchup with these fixes.

Comment 16

[Les Orchard [:lorchard]]

Hmm, I'm guessing that the database changes that go along with the patch have not been applied by IT.  I'm not sure who's doing that for bandwagon... do we have a bug for that, or a regular process for that?

https://wiki.mozilla.org/Update:Developers/Database_Changes

Comment 17

[David McNamara [:mackers]]

FWIW, currently the API is broken.

The service document gives my list of collections as normal, but they are without hrefs:

<collection href="collections//" ...

This means the extension will not display any feeds. 

It also means I'm stuck until this is fixed :)

Comment 18

[Les Orchard [:lorchard]]

Probably the same issue with pending DB changes :/

Comment 19

[Justin Scott [:fligtar]]

(In reply to comment #16)
> Hmm, I'm guessing that the database changes that go along with the patch have
> not been applied by IT.  I'm not sure who's doing that for bandwagon... do we
> have a bug for that, or a regular process for that?
>

We've just been filing IT bugs for it. If the changes are safe to be run in production as well, please specify that they should be run in production and on the bandwagon staging db.

Comment 20

[Les Orchard [:lorchard]]

FWIW, bug 484842 is tracking the DB changes

Comment 21

[Les Orchard [:lorchard]]

bug 484842 appears closed, and things seem to be working in the API and on the collections web page.  But, they seem to be pretty aggressively cached by the frontends, so requests may return stale broken content for awhile.  

You can forcibly defeat the caching by adding a unique value to a URL, like ?__=12342134 where the number could be the current time

Comment 22

[Brian King [:kinger]]

(In reply to comment #21)
> bug 484842 appears closed, and things seem to be working in the API and on the
> collections web page.  But, they seem to be pretty aggressively cached by the
> frontends, so requests may return stale broken content for awhile.  

Yes, we noticed this before and alerted Justin about it. On production we'll probably need this turned off. I'm not sure what the cache story will be when we go live.

> You can forcibly defeat the caching by adding a unique value to a URL, like
> ?__=12342134 where the number could be the current time

Thanks for the tip.

Comment 23

[David McNamara [:mackers]]

(In reply to comment #21)
> You can forcibly defeat the caching by adding a unique value to a URL, like
> ?__=12342134 where the number could be the current time

I have added this functionality to the extension.

If the BANDWAGON_RPC_ENABLE_CACHE_BUSTER constant in rpc/constants.js is positive, the above cache defeating method will take effect.

Revision 23712.

It is currently enabled.

Comment 24

[David McNamara [:mackers]]

What's outstanding:

> 4) In right panel, the header needs to be expanded to include an icon and a
> description if the subscription
> 5) Publisher Comments integration

Still waiting on these (bug 470245).

> 6) Special treatment for auto-publisher subscriptions at the top of the listing
in the left panel.

Done.

Comment 25

[David McNamara [:mackers]]

I think everything here is complete.

Comment 26

[Justin Scott [:fligtar]]

We're still missing the publisher "by" line under the date added. We replaced the "Category" part with that, and only when the add-on is selected (see mockup). I also noticed that the category text is still in the l10n file.

Comment 27

[Justin Scott [:fligtar]]

I added bandwagon.addon.addedby so that l10n will have this new string regardless of when this is implemented. I left the category string in so nothing would break, but it can be deleted when category is removed.

Comment 28

[Justin Scott [:fligtar]]

Moving to revised M5 milestone (4/28).

Comment 29

[David McNamara [:mackers]]

(In reply to comment #26)
> We're still missing the publisher "by" line under the date added. 

I have added the byline. I also made some small aesthetic changes to the addon display to bring it in line with the mockup.

r24532.