Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand add-on metadata returned by disco pane API #6546

Closed
jvillalobos opened this issue Apr 2, 2019 · 14 comments · Fixed by mozilla/addons-server#11274
Closed

Expand add-on metadata returned by disco pane API #6546

jvillalobos opened this issue Apr 2, 2019 · 14 comments · Fixed by mozilla/addons-server#11274
Assignees
Milestone

Comments

@jvillalobos
Copy link

Recommendation cards in the future will look more like the Universal Extension Card design:
extension-card
This will require the recommendations API for disco pane to return more add-on metadata, specifically: author, average rating, and average ADI.

@jvillalobos
Copy link
Author

This is part of the project for integrating disco pane into about:addons.

@diox
Copy link
Member

diox commented Apr 2, 2019

Adding that extra metadata should be just a matter of adding authors, ratings and average_daily_users to our DiscoveryAddonSerializer fields, I think, so fairly straightforward. At some point we'll need to change the way we generate heading and description text to match that design though.

@jvillalobos
Copy link
Author

Would they also need to be new fields, to keep the existing ones and preserve backward compatibility?

Sidenote: ideally we should avoid sending markup on those fields, so the client can rearrange and format them as needed.

@Rob--W
Copy link
Member

Rob--W commented Apr 24, 2019

When will this change land at AMO, or at least in which shape?

I'm working on the in-tree discopane at https://bugzilla.mozilla.org/show_bug.cgi?id=1546248 and need the information that Mathieu described in #6546

And indeed, I also need the editorial title and description without HTML. Currently I am post-processing the result to get the information, like this:

    // AMO API returns HTML, so strip it via AddonRepository._parseAddon.
    details.addon.summary = details.heading.split(" <span>")[0] + ".";
    details.addon.description = details.description;
    details.addon.authors = [{
      name: details.heading.match(/<a[^>]+>(?:.*by)?([^<]+)<\/a>/)[1],
      url: "https://example.com",
    }];
    let repositoryAddon = AddonRepository._parseAddon(details.addon);
    this.editorialHeading = repositoryAddon.description; // .summary
    this.editorialDescription = repositoryAddon.fullDescription; // .description

... but ideally I shouldn't do that.

Note that I added punctuation at the end of the editorial title because the information is rendered like <strong>editorial title that ends with period.</strong> ... rest of editorial description ... . Is it possible to return full sentences from the API, or should I always add a separator myself?

@diox diox self-assigned this Apr 24, 2019
@diox
Copy link
Member

diox commented Apr 24, 2019

The editorial heading & description are really meant to be consumed by the current disco frontend and nothing else, that's why the text currently doesn't end with a period. It's also translated, so I don't think adding punctuation yourself like that is a good idea.

Maybe the solution would be to have the API return a field with both heading and description mixed together in a single field, but I'd expect that field to continue to contain some limited HTML then.

@diox diox added this to the 2019.05.09 milestone Apr 24, 2019
@diox
Copy link
Member

diox commented Apr 24, 2019

After having given this more thought, separate properties are probably best, so I'll make that available for you together with the extra metadata.

More detailed explanation for those following at home:
The non HTML heading as entered by editorial team (i.e. Scott) looks like this:

Something something {start_sub_heading}with {addon_name}{end_sub_heading}

We then do some search & replace and return the HTML.

So I propose the following:

  • I modify the API to return an additional property containing the heading & description as entered by the editorial team (see above), with the special {} values replaced, but without HTML and without any transformations to add the author information. It will not contain HTML.
  • If those properties are empty, that means the editorial team entered the add-on in the admin tool but didn't enter any custom heading & description. The add-on original name and summary will be returned instead (or maybe I can just add summary to the fields returned in the addon object, leaving you to take care of that yourself in the client)
  • I add the extra properties requested above as well
  • You don't add punctuation, it should be added by the editorial team

@Rob--W
Copy link
Member

Rob--W commented Apr 24, 2019

To clarify, what you call "heading" in your comment is not actually the heading in the universal add-on card (see screenshot at the top of the issue), but the boldfaced text before the description, right?

The card is filled like this:

  • Icon at left (if extension), screenshot (if theme).
  • name at top
  • author name + link to AMO listing below name (yes, AMO listing, not author page)
  • Paragraph starts with editorial title if any (boldfaced), followed by editorial description.
  • If editorial text is missing, the short description (summary) is used instead.
  • Final row shows user ratings and user counts.

The last two are only shown for extensions, not themes.

I emphasized the parts of the card that need to be fetched from the API. If I understood you correctly, then what you described covers my needs.

@diox
Copy link
Member

diox commented Apr 25, 2019

To clarify, what you call "heading" in your comment is not actually the heading in the universal add-on card (see screenshot at the top of the issue), but the boldfaced text before the description, right?

Yes, confusing vocabulary because that's how we call it internally :)

@diox
Copy link
Member

diox commented Apr 25, 2019

@Rob--W note: you should use https://services.addons.mozilla.org as the domain to be consistent with other uses of our API in Firefox.

@Rob--W
Copy link
Member

Rob--W commented Apr 25, 2019

@diox Thanks for the information.

FYI I filed this BMO ticket to track integration of the updated discovery API in the in-tree discopane: https://bugzilla.mozilla.org/show_bug.cgi?id=1546980

@diox
Copy link
Member

diox commented Apr 29, 2019

@Rob--W this will land on Thursday, dev (http://addons-dev.allizom.org) will get it shortly, stage (https://addons.allizom.org) on Tuesday. Docs should have been updated (keep using "v4" API - we intend to freeze it soonish).

@Rob--W
Copy link
Member

Rob--W commented May 3, 2019

It's Friday and although addons-dev has the new API, stage does not.

I saw that a month ago it was announced that there won't be a push on May, 2nd (yesterday).
Will the new API land next Thursday (May, 9th)?

@diox
Copy link
Member

diox commented May 3, 2019

Yes, sorry, totally forgot about that. The milestone is correct, it will land on May 9th, and be available on stage on May 7th.

@AlexandraMoga
Copy link

The discovery api includes the new required metadata. There are also no regressions noted in the discovery pane.
I'm marking the issue as verified fixed on -dev.

@KevinMind KevinMind transferred this issue from mozilla/addons-server May 4, 2024
@KevinMind KevinMind added repository:addons-server Issue relating to addons-server migration:2024 labels May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants