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: 4663 - use the latest password for background tasks + refactoring #4869

Merged
merged 6 commits into from
Dec 21, 2023

Conversation

monsieurtanuki
Copy link
Contributor

@monsieurtanuki monsieurtanuki commented Dec 1, 2023

What

  • The main goal of this PR is to use the latest password possible for background tasks.
  • Typical use case:
    • a user created a background task (e.g. changed a product name)
    • the connection or the server was down, the background task was therefore not sent
    • and then the user changed the password
    • the background task should use the latest password instead of an old one - that's dealt with in this PR
  • Meanwhile, it was a good opportunity to refactor background tasks, that are all created using the same country (ProductQuery.getCountry()) and the same user ((ProductQuery.getUser()) and most of the time the same language (ProductQuery.getLanguage()) (except for image uploads that do have a specific language - "this is the FRONT image for DE")
  • This PR doesn't solve all the cases evoked in An issue that may cause errors in background tasks #4663 but only the low hanging fruit (and the most typical case).

Part of

Impacted files

  • background_task.dart: use the latest password; now we compute user and country from that class, and language most of the time too
  • background_task_barcode.dart: removed user and country parameters; replaced language
  • background_task_crop.dart: removed user and country parameters; replaced language
  • background_task_details.dart: removed user, language and country parameters
  • background_task_download_products.dart: removed user, language and country parameters
  • background_task_full_refresh.dart: removed user, language and country parameters
  • background_task_hunger_games.dart: removed user, language and country parameters
  • background_task_image.dart: removed user and country parameters; replaced language
  • background_task_offline.dart: removed user, language and country parameters
  • background_task_paged.dart: removed user, language and country parameters
  • background_task_progressing.dart: removed user, language and country parameters
  • background_task_refresh_later.dart: removed user, language and country parameters
  • background_task_top_barcodes.dart: removed user, language and country parameters
  • background_task_unselect.dart: removed user, language and country parameters
  • background_task_upload.dart: removed user and country parameters; replaced language

Impacted files:
* `background_task.dart`: use the latest password; now we compute user and country from that class, and language most of the time too
* `background_task_barcode.dart`: removed user and country parameters; replaced language
* `background_task_crop.dart`: removed user and country parameters; replaced language
* `background_task_details.dart`: removed user, language and country parameters
* `background_task_download_products.dart`: removed user, language and country parameters
* `background_task_full_refresh.dart`: removed user, language and country parameters
* `background_task_hunger_games.dart`: removed user, language and country parameters
* `background_task_image.dart`: removed user and country parameters; replaced language
* `background_task_offline.dart`: removed user, language and country parameters
* `background_task_paged.dart`: removed user, language and country parameters
* `background_task_progressing.dart`: removed user, language and country parameters
* `background_task_refresh_later.dart`: removed user, language and country parameters
* `background_task_top_barcodes.dart`: removed user, language and country parameters
* `background_task_unselect.dart`: removed user, language and country parameters
* `background_task_upload.dart`: removed user and country parameters; replaced language
* `paged_to_be_completed_product_query.dart`: minor refactoring
* `paged_user_product_query.dart`: minor refactoring
@g123k
Copy link
Collaborator

g123k commented Dec 4, 2023

The approach with if (storedUser.userId == currentUser.userId) … change password seems OK.
But it won't totally fix the issue, with this kind of flow:

User A : New background task
User A : new password + new login in the app
User B : login in the app

In that particular case, the background task won't use the latest password from User A.
Instead, I would "simply do": when a user logs in, iterate in all pending tasks and update the password if this is the relevant user.

Such behavior would allow notifying the user when we have some incorrect password error and quickly fix all issues

@monsieurtanuki
Copy link
Contributor Author

@g123k As stated in the description, this PR only solves one typical case (a user changes passwords).
Besides, the numerous sentry logs we got were due to the fact that the server does not support utf8 passwords for PATCH requests (used for packagings only).

I don't think it would make much sense to devote time to the unlikely situations you're describing, at least before the more likely (but not frequent either) "user-changing-passwords" case is dealt with, in this PR.

@github-actions github-actions bot removed the 👥 User management Account login, signup, signout label Dec 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (e5757e7) 9.67% compared to head (1a79f4e) 9.69%.

Files Patch % Lines
...ges/smooth_app/lib/background/background_task.dart 0.00% 10 Missing ⚠️
...th_app/lib/background/background_task_barcode.dart 0.00% 1 Missing ⚠️
...mooth_app/lib/background/background_task_crop.dart 0.00% 1 Missing ⚠️
...th_app/lib/background/background_task_details.dart 0.00% 1 Missing ⚠️
.../background/background_task_download_products.dart 0.00% 1 Missing ⚠️
...p/lib/background/background_task_hunger_games.dart 0.00% 1 Missing ⚠️
...ooth_app/lib/background/background_task_image.dart 0.00% 1 Missing ⚠️
...ooth_app/lib/background/background_task_paged.dart 0.00% 1 Missing ⚠️
...pp/lib/background/background_task_progressing.dart 0.00% 1 Missing ⚠️
.../lib/background/background_task_refresh_later.dart 0.00% 1 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #4869      +/-   ##
==========================================
+ Coverage     9.67%   9.69%   +0.01%     
==========================================
  Files          319     319              
  Lines        16147   16124      -23     
==========================================
  Hits          1563    1563              
+ Misses       14584   14561      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teolemon
Copy link
Member

teolemon commented Dec 8, 2023

I'm going to open an issue with the additional use-cases raised by @g123k

@monsieurtanuki
Copy link
Contributor Author

I'm going to open an issue with the additional use-cases raised by @g123k

Please do. I imagine the priority would be low, unless it's common for several users to use the same device and to log in / out accordingly.

@monsieurtanuki
Copy link
Contributor Author

@g123k Given that additional cases are dealt with in #4883, please consider reviewing the current PR.

@monsieurtanuki monsieurtanuki merged commit 28262ca into openfoodfacts:develop Dec 21, 2023
6 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you @g123k for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants