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

fix: Background image upload #2433

Merged
merged 132 commits into from
Sep 1, 2022

Conversation

AshAman999
Copy link
Member

@AshAman999 AshAman999 commented Jun 29, 2022

What

  • An initial implementation of using workmanager to do image upload in the background

Benefits

  • Background image upload
  • upload completes whenever there is good internet
  • once upload scheduled , will retry for five times before finally giving up
  • Task remains even when phone starts or app closed

Problems

  • can't easily manage app state when using workmanager [WIP] alternate ways to let users know if the task is done(possibly other menus)
  • ios setup is still not configured,(yet to be done,)

Fixes bug(s)

Part of

@AshAman999 AshAman999 requested a review from a team as a code owner June 29, 2022 19:47
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2022

Codecov Report

Merging #2433 (eba8d96) into develop (ac19a9c) will decrease coverage by 0.16%.
The diff coverage is 0.47%.

@@            Coverage Diff             @@
##           develop   #2433      +/-   ##
==========================================
- Coverage     7.04%   6.88%   -0.17%     
==========================================
  Files          219     221       +2     
  Lines        10874   11161     +287     
==========================================
+ Hits           766     768       +2     
- Misses       10108   10393     +285     
Impacted Files Coverage Δ
...th_app/lib/cards/data_cards/image_upload_card.dart 0.00% <0.00%> (ø)
...oth_app/lib/data_models/continuous_scan_model.dart 0.83% <0.00%> (ø)
...h_app/lib/data_models/onboarding_data_product.dart 0.00% <0.00%> (ø)
.../smooth_app/lib/data_models/onboarding_loader.dart 0.00% <ø> (ø)
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 15.70% <ø> (ø)
...smooth_app/lib/generic_lib/duration_constants.dart 0.00% <0.00%> (ø)
...smooth_app/lib/helpers/background_task_helper.dart 0.00% <0.00%> (ø)
...smooth_app/lib/helpers/picture_capture_helper.dart 0.00% <0.00%> (ø)
...e_panel/knowledge_panels/knowledge_panel_page.dart 0.00% <ø> (ø)
packages/smooth_app/lib/main.dart 14.52% <0.00%> (-0.13%) ⬇️
... and 18 more

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

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Thank you @AshAman999, that looks rather good.
I've never used workmanager before though, therefore there are things I may have missed.
Beyond my detailed comments, I have some questions:

  • I'm not convinced by the increasing delay (what delay is that btw? the delay from the moment I clicked on upload or from the latest unsuccessful attempt?). I guess a call to sentry would make sense ("hey we've got this guy and we cannot upload his picture"), as would an alert to the end-user.
  • Wouldn't it be nice to display somewhere the list of the pending tasks?
  • How do you refresh the app when the picture is uploaded? I saw something about the carousel, but I don't think it would refresh a product page, would it?

@teolemon
Copy link
Member

are there any outstanding issues left @AshAman999 @g123k ?

@AshAman999
Copy link
Member Author

are there any outstanding issues left @AshAman999 @g123k ?

@teolemon
Though as of now, nothing is blocking, but still, I was thinking to create an internal release with the pr to have things tested out before we finally merge it to develop.
@g123k ?

Copy link
Collaborator

@g123k g123k left a comment

Choose a reason for hiding this comment

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

LGTM :)
We can now test the feature

@teolemon
Copy link
Member

teolemon commented Sep 1, 2022

Ok, going to merge

@teolemon teolemon merged commit cf4fa6a into openfoodfacts:develop Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment