[Rocketbar] Duplicate results should not appear

RESOLVED FIXED

Status

Firefox OS
Gaia::Search
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: kgrandon, Assigned: kgrandon)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [dependency:Marketplace] )

User Story

We should support de-duplication of results. The following use-cases should be handled:

1)
- Only one app matches search from all sources
- No issue here

2)
- No local app that matches search
- e.me app that matches search
- Marketplace app that matches search
- In this scenario, it might be best to make the assumption that the Marketplace app is likely the best experience and only offer that one (though there may be some outlier scenarios where this is not ideal)

3)
- Local app that matches search
- e.me app that matches search
- No Marketplace app that matches search
- In this scenario, it likely makes sense to only show the local app (whether it is packaged or hosted, it makes no difference, it is likely >= the e.me version)

4)
- Local app that matches search
- No e.me app that matches search
- Marketplace app that matches search
- This scenario is more difficult because the Marketplace app may be a better experience than the local app, so perhaps we want to show both (However, if we can detect that it is the same app, just show the local one)
Francis, in this scenario we may want some way to distinguish the app icons (you may already be planning this for Marketplace apps)

5)
- Local app that matches search
- e.me app that matches search
- Marketplace app that matches search
- I think this is the same as #4, the e.me app should just not be shown

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
We would like to de-dupe results from displaying in the rocketbar. The following are the cases we've identified so far:

Bookmarks and E.me
  De-dupe if the URL is the same.

Apps and Marketplace
  De-dupe if the manifestURL is the same.

Marketplace and E.me
  We will likely need to extend our manifest files to support this. The most likely solution for this one would be to augment the manifest with a URL pointing to the website. This way we can detect duplicates.
(Assignee)

Updated

4 years ago
Depends on: 952419
No longer blocks: 946452
Component: Gaia::System → Gaia::Search
No longer depends on: 952419
Duplicate of this bug: 958826
(Assignee)

Updated

4 years ago
Depends on: 952419
(Assignee)

Updated

4 years ago
Summary: [Rocketbar] Duplicate results should not appear → [meta][Rocketbar] Duplicate results should not appear
(In reply to Kevin Grandon :kgrandon from comment #0)

> Marketplace and E.me
>   We will likely need to extend our manifest files to support this. The most
> likely solution for this one would be to augment the manifest with a URL
> pointing to the website. This way we can detect duplicates.

What do you need that is not provided by the launch_path?
(Assignee)

Comment 3

4 years ago
(In reply to Fabrice Desré [:fabrice] from comment #2)
> What do you need that is not provided by the launch_path?

We may be able to do something simple with the information we have for 1.4. In the long run, I think we'll need to do something more complex. I think there are often times where we may have mis-matching origins. I'm sure there's going to be quite a few edge-cases that launch_path won't cover.

One example today is if you search for 'facebook', you get two results, one from Marketplace and one from Everything.me.

Everything.me URL: touch.facebook.com 
Marketplace Origin: m.facebook.com
To summarize an email thread on this subject, there are a few key use cases:

1)
- Only one app matches search from all sources
- No issue here

2)
- No local app that matches search
- e.me app that matches search
- Marketplace app that matches search
- In this scenario, it might be best to make the assumption that the Marketplace app is likely the best experience and only offer that one (though there may be some outlier scenarios where this is not ideal)

3)
- Local app that matches search
- e.me app that matches search
- No Marketplace app that matches search
- In this scenario, it likely makes sense to only show the local app (whether it is packaged or hosted, it makes no difference, it is likely >= the e.me version)

4)
- Local app that matches search
- No e.me app that matches search
- Marketplace app that matches search
- This scenario is more difficult because the Marketplace app may be a better experience than the local app, so perhaps we want to show both (However, if we can detect that it is the same app, just show the local one)
Francis, in this scenario we may want some way to distinguish the app icons (you may already be planning this for Marketplace apps)

5)
- Local app that matches search
- e.me app that matches search
- Marketplace app that matches search
- I think this is the same as #4, the e.me app should just not be shown
Flags: needinfo?(fdjabri)
(Assignee)

Comment 5

4 years ago
Thanks for the great summary Peter. Since it appears we may keep this bug around, I've updated the `user story` section of this bug so the use-cases are more visible.
User Story: (updated)

Updated

4 years ago
Blocks: 946452
(Assignee)

Updated

4 years ago
Duplicate of this bug: 966993
(Assignee)

Updated

4 years ago
Depends on: 969308
(Assignee)

Updated

4 years ago
No longer depends on: 969308
(Assignee)

Updated

4 years ago
Duplicate of this bug: 952419
(Assignee)

Updated

4 years ago
Summary: [meta][Rocketbar] Duplicate results should not appear → [Rocketbar] Duplicate results should not appear
(Assignee)

Comment 8

4 years ago
Created attachment 8376500 [details] [review]
Github pull request
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
(Assignee)

Comment 9

4 years ago
Comment on attachment 8376500 [details] [review]
Github pull request

This is ready for a first review pass. Essentially this tries to do something similar as to what we do in Everything.me today, except we dedupe via JS logic before rendering. It's not as nice as specifying a mobile site in the manifest, and I do think we should follow that approach in the future - but that would require quite a lot of evangelism work to get that working properly in a 1.4 timeline.

Dale's suggestion has been taken here to have a collect method in the base search class that collects search results from each provider before rendering. 

I tried this with a variety of apps/sites and could not find a case that this does not work for. (Though it would be possible to create such a case with something like: manifest url: mozilla.org/manifest.webapp, site url: firefox.com. There is no way to detect something like this currently, without a hard-coded map.
Attachment #8376500 - Flags: review?(ran)
Attachment #8376500 - Flags: review?(dale)
Attachment #8376500 - Flags: review?(amirn)
Attachment #8376500 - Flags: feedback?(fabrice)
(Assignee)

Updated

4 years ago
Attachment #8376500 - Attachment description: WIP - Github pull request → Github pull request
Kevin, Pter, I have a suggestion on how to better dedup Local-marketplace app - have the marketplace api filter out results that have already been installed.

Advantages:
1. No need for client side deduping. You'll never get already installed results.
2. Marketplace suggestions will always display. Currently, installing an app decrements marketplace results for related searches.

This can be potentially done in two ways:
1. Concatenate installed app slugs to marketplace api request.
2. If user is signed in, marketplace api can do it on it's own.

Just a thought!
Argh! Typos... Wish I edit my own comment :'(
(that one was on purpose..)
I would like to suggest an alternative to the 'collect' approach.
Since the providers are already aware of the global 'Search' object, the following is simpler approach IMHO:

// in provider.js
search: function(input) {
  // search logic
  var deduped = Search.dedupe(this, results, /* dedupeStrategy */);
  this.render(deduped);
}

I like this approach better since it keeps the providers in charge of rendering and does not require passing the 'collect' method to each provider, and the 'dedupes' flag will not be necessary.
(Assignee)

Comment 14

4 years ago
(In reply to Ran Ben Aharon (Everything.me) from comment #10)
> Kevin, Pter, I have a suggestion on how to better dedup Local-marketplace
> app - have the marketplace api filter out results that have already been
> installed.

I do like the idea of trying something *like* this, but I'm not sure it will perform well. Sending all of those slugs over the wire in a request is going to add to the latency of them - we recently had a perf test when we had to test the homescreen with 10,000 installed applications. Also marketplace/local apps seem to easy to dedupe unless I am missing something - as we can dedupe them by a single field. Anyway - I think that maybe this could be an investigation/optimization after we land?


(In reply to Amir Nissim (Everything.me) from comment #13)
> I would like to suggest an alternative to the 'collect' approach.
> Since the providers are already aware of the global 'Search' object, the
> following is simpler approach IMHO:
> 
> // in provider.js
> search: function(input) {
>   // search logic
>   var deduped = Search.dedupe(this, results, /* dedupeStrategy */);
>   this.render(deduped);
> }
> 
> I like this approach better since it keeps the providers in charge of
> rendering and does not require passing the 'collect' method to each
> provider, and the 'dedupes' flag will not be necessary.

We can try something like this, I was trying to avoid having global access where possible. Also we will likely need an asynchronous collection point - so we can guarantee ordering in the search controller down the line. (We will probably need to guarantee that we process marketplace results before E.me for example).
(Assignee)

Comment 15

4 years ago
Updated the pull request to address some comments and also be a bit smarter. Additional test cases included as well.
Comment on attachment 8376500 [details] [review]
Github pull request

I'll let Amir speak on my behalf this time
Attachment #8376500 - Flags: review?(ran)
Comment on attachment 8376500 [details] [review]
Github pull request

I don't have a strong opinion on the implementation, but I'm pretty sure we want that to run client side as much as possible. There's also no doubt that this will need refinements but if the overall architecture is sound, that's fine.
Attachment #8376500 - Flags: feedback?(fabrice) → feedback+
Comment on attachment 8376500 [details] [review]
Github pull request

I raised my concerns in Comment 13, but don't want to block on it so r+
Attachment #8376500 - Flags: review?(amirn)
Attachment #8376500 - Flags: review+
Attachment #8376500 - Flags: feedback?

Updated

4 years ago
Attachment #8376500 - Flags: feedback? → feedback+
(Assignee)

Comment 19

4 years ago
Comment on attachment 8376500 [details] [review]
Github pull request

I'm sure we will find edge cases, but let's land for now and we can always follow-up. Thanks all!
Attachment #8376500 - Flags: review?(dale)
Flags: needinfo?(fdjabri)
(Assignee)

Comment 20

4 years ago
Landed: https://github.com/mozilla-b2g/gaia/commit/3bad231f0ecb9b75f74c1f426332047e43411058
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Hi, had to reopen this bug because had to revert this change in https://github.com/mozilla-b2g/gaia/commit/b88eb63c3950ecaa88d00399735e52d83ab6e307 because of test failures like 
https://tbpl.mozilla.org/php/getParsedLog.php?id=35017167&tree=B2g-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

4 years ago
Created attachment 8380351 [details] [review]
Pull request - fix test path

This is an updated pull request that copies mock_l10n into the search app (though this should probably go into shared/).

I think the backout here was due to the fact that we run tests differently on travis than tbpl.
Attachment #8380351 - Flags: review+
(Assignee)

Comment 23

4 years ago
Landing again with fixed tests: https://github.com/mozilla-b2g/gaia/commit/6bca90fae87994419e146e4b4a88ead90f3198f8
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [dependency:Marketplace]
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
Tracking bug 1026887 as marketplace (as a search term itself) is an edge case.
You need to log in before you can comment on or make changes to this bug.