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

feat: New system to show how well products match user preferences #6764

Merged
merged 15 commits into from
May 18, 2022

Conversation

stephanegigandet
Copy link
Contributor

This is an implementation of the product scoring system described in https://forum.openfoodfacts.org/t/proposal-for-a-new-personalized-experience-in-the-open-food-facts-app-and-website/71

  • Scores are now on a 0 to 100 scale

  • Product match status is now either:

  1. Very good match
  2. Good match
  3. Poor match
  4. Unknown match
    and if the user selected some hard restrictions (e.g. vegan, or allergens) (currently selected by marking the attribute mandatory, but we could make new settings specific for restrictions that would be binary "I'm allergic to gluten"):
  5. May not match (e.g. there can be traces of gluten, or some ingredient might not be vegan)
  6. Does not match (contains gluten, is not vegan).
  • Product match status is now indicated at the top of the card, in a similar way to the new Flutter app.
    The score is now displayed, but we could hide it. I display it currently for debugging purposes.

  • I removed the pagination at the top, it's now only on the bottom.

  • I removed the tabs for each match status, to make the interface cleaner.

Screenshot:
image

2 minute video demo:
https://www.loom.com/share/33ca5a6179bd49b29ad2404a1b1af2ff

@stephanegigandet stephanegigandet requested a review from a team as a code owner May 12, 2022 13:20
@github-actions github-actions bot added Attributes https://wiki.openfoodfacts.org/Product_Attributes CSS Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. Translations We use a non-standard version of GetText, lack language variants support translate.openfoodfacts.org labels May 12, 2022
@stephanegigandet stephanegigandet changed the title New system to show how well products match user preferences feat: New system to show how well products match user preferences May 12, 2022
Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me there are potential issues with status computation.

Comment on lines 84 to 85
else if ((attribute_preference == "mandatory") && (attribute.match <= 50)
&& ((status = "match") || (status = "may-not-match"))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean ===, don't you ?

Where does the "may-not-match" comes from ?

html/js/product-search.js Show resolved Hide resolved
@@ -16,7 +16,7 @@
function match_product_to_preferences (product, product_preferences) {

var score = 0;
var status = "yes";
var status = "compatible";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document possible values of status here.

Comment on lines 120 to 122
else if (status == "does_not_match") {
score = 0;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do this above (as suggested) for this is not logical to have it here…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Comment on lines 100 to 101
// Normalize the score from 0 to 100
score /= sum_of_factors;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Normalize the score from 0 to 100
score /= sum_of_factors;
if (status !== "does_not_match") {
// Normalize the score from 0 to 100
score /= sum_of_factors;
else {
score = 0;
}

Copy link
Member

@alexgarel alexgarel May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm not sure why we don't have "may_not_match" and "unknown_match" in the condition.

Comment on lines 212 to 214
product_html += '<div class="list_product_banner list_product_banner_' + product.match_status + '">'
+ lang()["products_match_" + product.match_status] + ' ' + Math.round(product.match_score) + '/100</div>'
+ '<div class="list_product_content">';
Copy link
Member

@alexgarel alexgarel May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal: why did you remove the use of javascript template ? it's less readable IMO.

@@ -813,7 +813,7 @@ sub compute_attribute_nova($$) {
$match = 100;
}
elsif ($nova_group == 3) {
$match = 50;
$match = 75;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: This part could be better written as a lookup in a hash with scores.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


products.sort(function(a, b) {
return (b.match_score - a.match_score) || (a.initial_order - b.initial_order);
return (b.match_score - a.match_score) // Highest score first
|| ((a.match_status == "does_not_match" ? 1 : 0) - (b.match_status == "does_not_match" ? 1 : 0)) // Matching products second
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use === for matching in js.
Why "does_not_match" gives me 1 point instead of 0 ?

Copy link
Member

@alexgarel alexgarel May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also is it normal that we do not consider the other status like may_not_match ?

Copy link
Member

@alexgarel alexgarel May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum I see, it's because you reversed a and b order compared to above expression.

I would better have:

((b.match_status == "does_not_match" ? 0 : 1) - (a.match_status == "does_not_match" ? 0 : 1)) // Matching products second

but it's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll change, and use === instead of ==

@alexgarel
Copy link
Member

I removed the tabs for each match status, to make the interface cleaner.

@stephanegigandet we could add a selector on the top to hide some status (eg. unknown etc)… but for a future version ;-)

@alexgarel
Copy link
Member

@stephanegigandet

  1. I'm not sure the /100 needs to be displayed, it makes even more data for eyes to process, without any information gain (maybe a % is ok, or nothing ?)

  2. color choices do not really feat for color blind people. They have the text though (so it's already functional), maybe use a ligther color for unknown match ?
    Capture d’écran de 2022-05-17 12-12-41

@stephanegigandet
Copy link
Contributor Author

  1. I'm not sure the /100 needs to be displayed, it makes even more data for eyes to process, without any information gain (maybe a % is ok, or nothing ?)

Right, in fact it's mostly for debugging purposes, I'm not sure we will show the score at all in the final version, maybe just the status.

  1. color choices do not really feat for color blind people. They have the text though (so it's already functional), maybe use a ligther color for unknown match ?

Sure, we can put a lighter grey.

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, just a potential bug, that is better to fix ;-)

html/js/product-search.js Outdated Show resolved Hide resolved
}
else {
product.match_status = "poor_match";
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool refactor 👍

stephanegigandet and others added 2 commits May 18, 2022 15:28
Co-authored-by: Alex Garel <alex@garel.org>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@stephanegigandet stephanegigandet merged commit 6749369 into main May 18, 2022
@stephanegigandet stephanegigandet deleted the product-match branch May 18, 2022 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Attributes https://wiki.openfoodfacts.org/Product_Attributes CSS Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. 🧪 tests Translations We use a non-standard version of GetText, lack language variants support translate.openfoodfacts.org
Projects
Development

Successfully merging this pull request may close these issues.

2 participants