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: Filter insights opportunities based on type of tags #859

Merged
merged 26 commits into from
Sep 12, 2022

Conversation

Jagrutiti
Copy link
Member

@Jagrutiti Jagrutiti commented Aug 15, 2022

What

  • This PR filters insight based on the number of questions unanswered, filtered by tags.

Why

Fixes bug(s)

@Jagrutiti Jagrutiti requested a review from a team as a code owner August 15, 2022 04:23
params={
"count": 5,
"page": 1,
"question_type": "category",
Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @alexgarel

I am not able to pass the parameters to the functions. question_type is always None when passed to get_insights

Copy link
Member

Choose a reason for hiding this comment

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

It's working, but I think you pdb in the first simulate_get above at line 576, where this parameter is None ! (that's why this is a good idea to separate tests ;-) )

@teolemon teolemon changed the title Filter insights opportunities based on type of tags feat: Filter insights opportunities based on type of tags Aug 22, 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.

@Jagrutiti some comments to put you back on track.

robotoff/app/api.py Outdated Show resolved Hide resolved
insights = list(
get_insights(
keep_types=[question_type],
group_by=True,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good option to have a boolean there.

So either we put a string, the field we want to group on: here group_by=Insight.value_tag (which as the draw back of using Insight)

or we call the parameter group_by_value_tag (which is a bit less elegant but may better do the job here).

robotoff/app/core.py Outdated Show resolved Hide resolved
tests/integration/models_utils.py Outdated Show resolved Hide resolved
tests/integration/test_api.py Outdated Show resolved Hide resolved
robotoff/app/api.py Outdated Show resolved Hide resolved
params={
"count": 5,
"page": 1,
"question_type": "category",
Copy link
Member

Choose a reason for hiding this comment

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

It's working, but I think you pdb in the first simulate_get above at line 576, where this parameter is None ! (that's why this is a good idea to separate tests ;-) )

tests/integration/test_api.py Outdated Show resolved Hide resolved
Comment on lines +121 to +147
product1 = ProductInsightFactory(type="category", value_tag="en:beer")
insight_data1 = get_insights(keep_types=["category"])
insight_data_items1 = [item.to_dict() for item in insight_data1]
assert insight_data_items1[0]["id"] == product1.id
# assert insight_data_items1[0]["type"] == []

product2 = ProductInsightFactory(type="label", value_tag="en:apricots")
insight_data2 = get_insights(keep_types=["label"], value_tag="en:apricots")
insight_data_items2 = [item.to_dict() for item in insight_data2]
assert insight_data_items2[0]["id"] == product2.id
# assert insight_data_items2[0]["type"] == []

product3 = ProductInsightFactory(type="label", value_tag="en:soups")
insight_data3 = get_insights(keep_types=["label"], value_tag="en:soups")
insight_data_items3 = [item.to_dict() for item in insight_data3]
assert insight_data_items3[0]["id"] == product3.id
# assert insight_data_items3[0]["type"] == []

product4 = ProductInsightFactory(type="category", value_tag="en:chicken")
insight_data4 = get_insights(keep_types=["category"], value_tag="en:chicken")
insight_data_items4 = [item.to_dict() for item in insight_data4]
assert insight_data_items4[0]["id"] == product4.id
# assert insight_data_items4[0]["type"] == []

insight_data5 = get_insights(keep_types=["category"])
insight_data_items5 = [item.to_dict() for item in insight_data5]
assert len(insight_data_items5) == 2
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 first create all your data, and then do one or two call to get the lists.

you need insight spread on:

  • at least two types
  • different value_tag (but with repeated values)
  • different barcodes

@alexgarel
Copy link
Member

@Jagrutiti thanks, you now only have to merge master before we can merge it back !

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #859 (7b09135) into master (1e63ca2) will increase coverage by 7.91%.
The diff coverage is 75.16%.

@@            Coverage Diff             @@
##           master     #859      +/-   ##
==========================================
+ Coverage   44.73%   52.65%   +7.91%     
==========================================
  Files          96       92       -4     
  Lines        6981     7014      +33     
==========================================
+ Hits         3123     3693     +570     
+ Misses       3858     3321     -537     
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/image_flag.py 41.02% <ø> (ø)
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%) ⬆️
... and 61 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sonarcloud
Copy link

sonarcloud bot commented Sep 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 6 Security Hotspots
Code Smell A 115 Code Smells

0.0% 0.0% Coverage
0.1% 0.1% Duplication

@alexgarel alexgarel merged commit bc5ee1b into master Sep 12, 2022
@alexgarel alexgarel deleted the get-unanswered-questions branch September 12, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants