feat: display insight/prediction for a product #815

Jagrutiti · 2022-06-26T15:28:21Z

Working on displaying insight/prediction etc to a product.

Following the steps listed in the description.
Currently: list all ProductInsight for this barcode - sort them in a predictable way : type - timestamp - id

Relates to

Display insight / predictions etc for a product or a particular filter #618

…t the data by timestamp

teolemon · 2022-06-27T15:10:03Z

def test_popular_question_pagination(client, mocker):
    mocker.patch("robotoff.insights.question.get_product", return_value={})
    ProductInsight.delete().execute()  # remove default sample
    for i in range(0, 12):
        ProductInsightFactory(barcode=i, unique_scans_n=100 - i)
    result = client.simulate_get("/api/v1/questions/popular?count=5&page=1")
    assert result.status_code == 200
    data = result.json
    assert data["count"] == 12
    assert data["status"] == "found"

  assert [q["barcode"] for q in data["questions"]] == ["0", "1", "2", "3", "4"]

E AssertionError: assert ['11', '10', '9', '8', '7'] == ['0', '1', '2', '3', '4']
E At index 0 diff: '11' != '0'
E Full diff:
E - ['0', '1', '2', '3', '4']
E + ['11', '10', '9', '8', '7']
tests/integration/test_api.py:102: AssertionError
---------------------------- Captured stdout setup ---------

…hub.com/openfoodfacts/robotoff into display-insight-productions-for-product

alexgarel

Here is my first review.

alexgarel · 2022-07-07T15:29:45Z

robotoff/app/core.py

+    query = query.order_by(ProductInsight.timestamp.desc())
+


you should add this only if we have a order_by == "most_recent" parameter, not always, because you are going to change existing code in an unpredictable manner.

This is what makes the test failed by the way (because your line removes previous ordering).

robotoff/app/api.py

…to display-insight-productions-for-product

alexgarel · 2022-07-11T09:18:31Z

robotoff/app/api.py

+        count: int = req.get_param_as_int("count", min_value=1, default=25)
+        keep_types: Optional[List[str]] = req.get_param_as_list(
+            "insight_types", required=False
+        )
+        barcode: Optional[str] = req.get_param("barcode")
+        value_tag: str = req.get_param("value_tag")
+        brands = req.get_param_as_list("brands") or None
+        server_domain: Optional[str] = req.get_param("server_domain")
+
+        if keep_types:
+            # Limit the number of types to prevent slow SQL queries
+            keep_types = keep_types[:10]
+
+        if brands is not None:
+            # Limit the number of brands to prevent slow SQL queries
+            brands = brands[:10]


I think we can factor this code between InsightCollection and DisplayInsightPredictionForProducts.

The idea, is that the function can return a dict (say query_params), and then simply use:
get_predictions = functools.partial(get_predictions, **query_params)

robotoff/app/api.py

tests/integration/test_api.py

robotoff/app/api.py

alexgarel

Here is to unblock you.

alexgarel · 2022-07-13T07:46:47Z

robotoff/app/api.py

+api.add_route(
+    "/api/v1/insights/predictions/display", DisplayInsightPredictionForProducts()
+)


Why not just "/api/v1/insights/predictions" to be coherent with the url for insights ?

robotoff/app/core.py

Co-authored-by: Alex Garel <[email protected]>

…hub.com/openfoodfacts/robotoff into display-insight-productions-for-product

codecov · 2022-07-13T14:59:05Z

Codecov Report

Merging #815 (c0d3817) into master (1e63ca2) will increase coverage by 6.74%.
The diff coverage is 70.44%.

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
+ Coverage   44.73%   51.47%   +6.74%     
==========================================
  Files          96       92       -4     
  Lines        6981     6840     -141     
==========================================
+ Hits         3123     3521     +398     
+ Misses       3858     3319     -539

Impacted Files	Coverage Δ
robotoff/cli/batch.py	`0.00% <ø> (ø)`
robotoff/cli/insights.py	`0.00% <0.00%> (ø)`
robotoff/insights/dataclass.py	`100.00% <ø> (ø)`
robotoff/prediction/ocr/brand.py	`68.62% <0.00%> (ø)`
robotoff/prediction/ocr/expiration_date.py	`25.71% <ø> (ø)`
robotoff/prediction/ocr/label.py	`72.30% <0.00%> (ø)`
robotoff/prediction/ocr/packager_code.py	`73.07% <ø> (ø)`
robotoff/prediction/ocr/product_weight.py	`49.10% <0.00%> (+1.28%)`	⬆️
robotoff/products.py	`42.75% <ø> (+2.26%)`	⬆️
robotoff/workers/listener.py	`0.00% <0.00%> (ø)`
... and 60 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

alexgarel

We have to add more tests:

Right now your test is just testing status_code, it should check response content
also you should then add a test where you create some predictions (PredictionFactory will help you do that) and check that it returns predictions to you
Also you should check some filters to see they are working
You should also add some tests on get_predictions (there again using PredictionFactory)

…to display-insight-productions-for-product

…hub.com/openfoodfacts/robotoff into display-insight-productions-for-product

robotoff/app/api.py

…hub.com/openfoodfacts/robotoff into display-insight-productions-for-product

tests/integration/test_api.py

alexgarel · 2022-07-19T17:13:51Z

tests/integration/test_api.py

+    PredictionFactory(barcode="123", keep_types="category", value_tag="en:seeds")
+    PredictionFactory(type="category")
+    PredictionFactory(value_tag="en:seeds")
+    PredictionFactory(type="brand")


I would like those creations to reflect the data proposal here : #815 (comment)

More over it's better to keep created predictions in variables (prediction1, prediction2…), so that you can check for ids in you test (first request you assert the fields, but then you just assert ids).

Co-authored-by: Alex Garel <[email protected]>

tests/integration/test_api.py

Co-authored-by: Alex Garel <[email protected]>

Jagrutiti · 2022-07-20T15:34:56Z

tests/integration/test_api.py

-    assert actual_items2[0]["barcode"] == "123"
-    assert actual_items2[0]["type"] == "brand"
-    assert actual_items2[0]["value_tag"] == "en:seeds"
+    assert prediction2.id == prediction1.id + 1


@alexgarel

I am not sure about this line though

you should not have to test that. First it's not related to your test (you are not supposed to be testing PredictionFactory here), and second it's not garanteed to be true.

alexgarel

Great @Jagrutiti, we are almost there, some small changes and it will be ok.

tests/integration/test_api.py

alexgarel · 2022-07-20T16:12:30Z

tests/integration/test_api.py

-    assert actual_items2[0]["barcode"] == "123"
-    assert actual_items2[0]["type"] == "brand"
-    assert actual_items2[0]["value_tag"] == "en:seeds"
+    assert prediction2.id == prediction1.id + 1


you should not have to test that. First it's not related to your test (you are not supposed to be testing PredictionFactory here), and second it's not garanteed to be true.

tests/integration/test_api.py

Co-authored-by: Alex Garel <[email protected]>

alexgarel

Ok let's merge it :-)

(I will just commit my suggestion first)

tests/integration/test_api.py

make result predictible by sorting them

sonarqubecloud · 2022-07-23T14:26:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
1 Security Hotspot
100 Code Smells

0.0% Coverage
0.1% Duplication

Added test case and to test insight prediction for product and to sor…

633b839

…t the data by timestamp

github-actions bot assigned Jagrutiti Jun 26, 2022

Format code with black and isort

26fc6a7

Jagrutiti added 2 commits June 28, 2022 19:57

Styling

5cb9084

Merge branch 'display-insight-productions-for-product' of https://git…

6158796

…hub.com/openfoodfacts/robotoff into display-insight-productions-for-product

alexgarel requested changes Jul 7, 2022

View reviewed changes

Jagrutiti added 2 commits July 9, 2022 10:42

Merge branch 'master' of https://github.com/openfoodfacts/robotoff in…

ed8ae4f

…to display-insight-productions-for-product

Wrote a new function to get_predictions

3659887

Jagrutiti marked this pull request as ready for review July 9, 2022 07:53

Jagrutiti requested a review from a team as a code owner July 9, 2022 07:53

alexgarel reviewed Jul 11, 2022

View reviewed changes

robotoff/app/api.py Outdated Show resolved Hide resolved

alexgarel reviewed Jul 11, 2022

View reviewed changes

tests/integration/test_api.py Outdated Show resolved Hide resolved

Jagrutiti and others added 3 commits July 11, 2022 22:36

Updating code and test cases

532547b

Updated the code and test case

ea05916

Format code with black and isort

3b09f79

alexgarel reviewed Jul 13, 2022

View reviewed changes

robotoff/app/api.py Outdated Show resolved Hide resolved

alexgarel requested changes Jul 13, 2022

View reviewed changes

Jagrutiti and others added 5 commits July 13, 2022 20:15

Update robotoff/app/core.py

cc899a3

Co-authored-by: Alex Garel <[email protected]>

Format code with black and isort

013f5c8

Updating the API URL and its class name

2070121

Merge branch 'display-insight-productions-for-product' of https://git…

b1a27b0

…hub.com/openfoodfacts/robotoff into display-insight-productions-for-product

Format code with black and isort

8976dab

alexgarel requested changes Jul 13, 2022

View reviewed changes

Jagrutiti added 2 commits July 14, 2022 20:16

Merge branch 'master' of https://github.com/openfoodfacts/robotoff in…

f30c0ea

…to display-insight-productions-for-product

Merge branch 'display-insight-productions-for-product' of https://git…

6cde8f1

…hub.com/openfoodfacts/robotoff into display-insight-productions-for-product

alexgarel reviewed Jul 15, 2022

View reviewed changes

robotoff/app/api.py Outdated Show resolved Hide resolved

alexgarel reviewed Jul 15, 2022

View reviewed changes

robotoff/app/api.py Show resolved Hide resolved

Jagrutiti added 4 commits July 18, 2022 22:39

Adding new scenarios to test case

5e8e182

Merge branch 'display-insight-productions-for-product' of https://git…

adc709b

…hub.com/openfoodfacts/robotoff into display-insight-productions-for-product

Merging revieq suggestions

ba4dd3b

Added new filters to test case

61a278e

Jagrutiti commented Jul 19, 2022

View reviewed changes

tests/integration/test_api.py Outdated Show resolved Hide resolved

alexgarel reviewed Jul 19, 2022

View reviewed changes

Jagrutiti and others added 2 commits July 20, 2022 19:58

Update tests/integration/test_api.py

04ef480

Co-authored-by: Alex Garel <[email protected]>

Fixing the test case

36b605e

alexgarel reviewed Jul 20, 2022

View reviewed changes

tests/integration/test_api.py Outdated Show resolved Hide resolved

Jagrutiti and others added 2 commits July 20, 2022 21:03

Update tests/integration/test_api.py

17dddb0

Co-authored-by: Alex Garel <[email protected]>

Format code with black and isort

c10c894

Jagrutiti commented Jul 20, 2022

View reviewed changes

alexgarel requested changes Jul 20, 2022

View reviewed changes

Jagrutiti and others added 5 commits July 20, 2022 22:03

Update tests/integration/test_api.py

718556c

Co-authored-by: Alex Garel <[email protected]>

Update tests/integration/test_api.py

1452498

Co-authored-by: Alex Garel <[email protected]>

Update tests/integration/test_api.py

a174b62

Co-authored-by: Alex Garel <[email protected]>

Update tests/integration/test_api.py

cadbaf5

Co-authored-by: Alex Garel <[email protected]>

Updating the test case

1621df6

alexgarel approved these changes Jul 21, 2022

View reviewed changes

tests/integration/test_api.py Show resolved Hide resolved

alexgarel added 4 commits July 21, 2022 10:07

test: small change after review

46aa7fb

Merge branch 'master' into display-insight-productions-for-product

b40f32b

test: fix after merge

85439b3

test: fix prediction collection test

8791743

make result predictible by sorting them

alexgarel force-pushed the display-insight-productions-for-product branch from 4aff732 to 8791743 Compare July 22, 2022 12:26

test: fix tests

c0d3817

alexgarel force-pushed the display-insight-productions-for-product branch from 1e80093 to c0d3817 Compare July 23, 2022 14:17

alexgarel merged commit a9c9be8 into master Jul 23, 2022

alexgarel deleted the display-insight-productions-for-product branch July 23, 2022 15:58

alexgarel mentioned this pull request Jul 23, 2022

chore(master): release 1.6.0 #766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: display insight/prediction for a product #815

feat: display insight/prediction for a product #815

Jagrutiti commented Jun 26, 2022 •

edited by alexgarel

Loading

teolemon commented Jun 27, 2022

alexgarel left a comment

alexgarel Jul 7, 2022

alexgarel Jul 11, 2022

alexgarel left a comment

alexgarel Jul 13, 2022

codecov bot commented Jul 13, 2022 •

edited

Loading

alexgarel left a comment

alexgarel Jul 19, 2022

Jagrutiti Jul 20, 2022

alexgarel Jul 20, 2022

alexgarel left a comment

alexgarel Jul 20, 2022

alexgarel left a comment •

edited by Jagrutiti

Loading

sonarqubecloud bot commented Jul 23, 2022

feat: display insight/prediction for a product #815

feat: display insight/prediction for a product #815

Conversation

Jagrutiti commented Jun 26, 2022 • edited by alexgarel Loading

Relates to

teolemon commented Jun 27, 2022

alexgarel left a comment

Choose a reason for hiding this comment

alexgarel Jul 7, 2022

Choose a reason for hiding this comment

alexgarel Jul 11, 2022

Choose a reason for hiding this comment

alexgarel left a comment

Choose a reason for hiding this comment

alexgarel Jul 13, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 13, 2022 • edited Loading

Codecov Report

alexgarel left a comment

Choose a reason for hiding this comment

alexgarel Jul 19, 2022

Choose a reason for hiding this comment

Jagrutiti Jul 20, 2022

Choose a reason for hiding this comment

alexgarel Jul 20, 2022

Choose a reason for hiding this comment

alexgarel left a comment

Choose a reason for hiding this comment

alexgarel Jul 20, 2022

Choose a reason for hiding this comment

alexgarel left a comment • edited by Jagrutiti Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 23, 2022

Jagrutiti commented Jun 26, 2022 •

edited by alexgarel

Loading

codecov bot commented Jul 13, 2022 •

edited

Loading

alexgarel left a comment •

edited by Jagrutiti

Loading