862603 - Remove Summary from apps

Status: VERIFIED FIXED

(Marketplace Graveyard :: General, defect, P3)

Description

[Matt Basta [:basta]]

- Most apps just copy/paste, so it's redundant
- We don't have summary in the search results anymore

To do:
- Remove the field from the DB
- Remove the field from the API
- Make the description field on the detail page a true expander for description (text-overflow: ellipsis)

Comment 1

[Rob Hudson [:robhudson]]

It's also indexed in elasticsearch. Although it might be used for add-ons on AMO.

Comment 2

[Matt Basta [:basta]]

Ewww, yeah, strike the first item from my To Do list. We don't want to break AMO. But we should probably still have a migration to strip summaries from existing apps.

Comment 3

[Wil Clouser [:clouserw]]

UX: is this something we talked about?

Comment 4

[Matt Basta [:basta]]

Though I'd love for this to be a fireplaceism, this isn't in the scope of Fireplace's mvp. We need to ship!!!

Comment 5

[Christopher Van Wiemeersch [:cvan]]

Maria, you okay with this proposed change?

Comment 6

[Maria Sandberg [:mushi]]

Makes sense to me, we aren't really surfacing the summary anywhere and it seems redundant if developers often copy paste there. I like removing things!

Comment 7

[Rob Hudson [:robhudson]]

I can take this. One question though... what do we do with the current summaries in the database? Should I simply copy the summary to the description? That is: description = CONCAT(summary, ' ', description)?

Comment 8

[Christopher Van Wiemeersch [:cvan]]

With a newline, sure!

Comment 9

[Matt Basta [:basta]]

I think the idea was to do a bit of a heuristic:

1. If description.startswith(summary), just delete the summary
2. If the description is empty, set the description to the summary
3. Otherwise CONCAT(summary, '<br>', description)

Comment 10

[Rob Hudson [:robhudson]]

(In reply to Matt Basta [:basta] from comment #9)
> I think the idea was to do a bit of a heuristic:
> 
> 1. If description.startswith(summary), just delete the summary
> 2. If the description is empty, set the description to the summary
> 3. Otherwise CONCAT(summary, '<br>', description)

That makes sense. Thanks basta.

Better to use '\n' than put HTML in the field? How is the field used/rendered? Do we accept/encourage HTML? Or do we do something like `nl2br` on output?

Should we notify developers that we've mucked with their descriptions?

Comment 12

[Rob Hudson [:robhudson]]

(In reply to Matt Basta [:basta] from comment #9)
> I think the idea was to do a bit of a heuristic:
> 
> 1. If description.startswith(summary), just delete the summary
> 2. If the description is empty, set the description to the summary
> 3. Otherwise CONCAT(summary, '<br>', description)

BTW, I have the above implemented currently in a pull request here:
https://github.com/mozilla/zamboni/pull/835/files#L17R449

Locales complicate this. Take, for example, the twitter app. They have a summary and description in "en-US". The summary is simply "Twitter for mobile" while the description is a full description.

Then they have the same summary translated into about 10 different locales but no description in those locales. What I would expect to see on our description pages in this case is the description in "en-US", or perhaps the concatenation of the localized summary and English description. However #2 would simply copy the localized summary to the description and miss the full description in English.

What's the proper way to handle the above case? I'm not sure a script could determine if a non-localized description is better than a localized summary? Or if copying the localized summary in this case is ok we can run with what I have.

Comment 13

[Matt Basta [:basta]]

Is it possible to run the heuristic from comment #9 per locale per app summary? I'm not sure if I understand you 100%, but ideally we should never have unlocalized content marked as being localized.

Comment 14

[Rob Hudson [:robhudson]]

(In reply to Matt Basta [:basta] from comment #13)
> Is it possible to run the heuristic from comment #9 per locale per app
> summary?

That's what I have in the patch currently.

> I'm not sure if I understand you 100%, but ideally we should never
> have unlocalized content marked as being localized.

For twitter it seemed as if the intention was to have the localized summary + full description, even non-localized. E.g. in "en-US" it's summary (en) + description (en). But since they don't have description in any other locale our db query would fall back to the app's default locale and find the description in English. So if my lang is "pt", currently I'd see summary (pt) + description (en) which is "Twitter para celular" + "<long description in english>". But after the migration I'd simply see "Twitter para celular", because I'd find no description in "pt" and copy the summary to the description.

Comment 15

[Matt Basta [:basta]]

I think the answer is to, in those cases, have the translated description be the translated summary and nothing else. We should be notifying the developers of this change through other channels and ask them to update their description as necessary.

Is it shitty that we're remove a big blob of description? Probably. But there's a few justifications:

1. We shouldn't put text from Locale X in a field marked as Locale Y
2. If a developer has a translated summary but not a translated description, the description probably isn't a high quality bit of text and probably got blindly copied from somewhere.
3. Users who care about the localized text won't have a degraded or improved experience since they probably couldn't read the English text anyway.

I think messaging is definitely important, though, and making sure developers are aware of this change is something we need to do. I'll file a bug about it.

Comment 16

[Rob Hudson [:robhudson]]

Thanks Matt. I just need a good r? on that code then and we can test it on -dev.

https://github.com/mozilla/zamboni/pull/835

Comment 17

[Rob Hudson [:robhudson]]

https://github.com/mozilla/zamboni/commit/c9542a1

Comment 18

[krupa raj[:krupa]]

verified fixed at 
https://marketplace-dev.allizom.org/app/twitter?lang=fr
https://marketplace-dev.allizom.org/app/twitter?lang=en

see post-fix screenshot

Comment 19

[krupa raj[:krupa]]

Attachment: post-fix screenshot