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: 4423 - specific "Not connected to internet" displayed error #4455

Merged
merged 14 commits into from
Aug 16, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • This PR is limited to the product refresh in the product page.
  • The current behavior is that the refresh opens a "working..." dialog, and opens a generic "error" dialog if something fails.
  • Now, if the refresh fails, we check the connectivity, and if the user is not connected to internet we display a specific message.
  • We could even refine the message by ping'ing the server, with a possible "server down" message.
  • And later this could be applied to the rest of the server queries (product list refresh for instance). But here we're also in competition with Rethink UI of history refresh not to occlude the view #4425.

Screenshot

Screenshot_2023-08-05-19-23-29

Part of

Impacted files

  • barcode_product_query.dart: removed useless try as already catch'ed
  • continuous_scan_model.dart: removed the codeInvalid case that could never happen
  • fetched_product.dart: refactored with explicit constructors and additional exception and connectivity fields; removed the codeInvalid case that could never happen
  • new_product_page.dart: minor refactoring
  • product_dialog_helper.dart: removed the codeInvalid case that could never happen; minor refactoting
  • product_list_item_simple.dart: removed the codeInvalid case that could never happen
  • product_loader_page.dart: removed useless try as already catch'ed
  • product_refresher.dart: added a specific "You're not connected to the internet" error message; refactored using more FetchedProduct; removed useless method
  • pubspec.lock: wtf
  • pubspec.yaml: added package connectivity_plus
  • question_card.dart: refactored using FetchedProduct

Impacted files:
* `barcode_product_query.dart`: removed useless `try` as already `catch`'ed
* `continuous_scan_model.dart`: removed the `codeInvalid` case that could never happen
* `fetched_product.dart`: refactored with explicit constructors and additional exception and connectivity fields; removed the `codeInvalid` case that could never happen
* `new_product_page.dart`: minor refactoring
* `product_dialog_helper.dart`: removed the `codeInvalid` case that could never happen; minor refactoting
* `product_list_item_simple.dart`: removed the `codeInvalid` case that could never happen
* `product_loader_page.dart`: removed useless `try` as already `catch`'ed
* `product_refresher.dart`: added a specific "You're not connected to the internet" error message; refactored using more `FetchedProduct`; removed useless method
* `pubspec.lock`: wtf
* `pubspec.yaml`: added package `connectivity_plus`
* `question_card.dart`: refactored using `FetchedProduct`
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner August 5, 2023 18:29
@github-actions github-actions bot added 🥫 Product page Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. dependencies Hunger Games labels Aug 5, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2023

Codecov Report

Merging #4455 (985440a) into develop (1c75f86) will increase coverage by 0.00%.
Report is 2 commits behind head on develop.
The diff coverage is 5.97%.

@@           Coverage Diff            @@
##           develop    #4455   +/-   ##
========================================
  Coverage    10.31%   10.32%           
========================================
  Files          296      296           
  Lines        15311    15328   +17     
========================================
+ Hits          1579     1582    +3     
- Misses       13732    13746   +14     
Files Changed Coverage Δ
...oth_app/lib/data_models/continuous_scan_model.dart 0.00% <ø> (ø)
...ooth_app/lib/pages/hunger_games/question_card.dart 0.00% <0.00%> (ø)
...ib/pages/product/common/product_dialog_helper.dart 0.00% <0.00%> (ø)
...pages/product/common/product_list_item_simple.dart 0.00% <ø> (ø)
...pp/lib/pages/product/common/product_refresher.dart 0.00% <0.00%> (-1.21%) ⬇️
...smooth_app/lib/pages/product/new_product_page.dart 0.46% <0.00%> (ø)
...oth_app/lib/pages/product/product_loader_page.dart 1.33% <0.00%> (-0.06%) ⬇️
...es/smooth_app/lib/query/barcode_product_query.dart 0.00% <0.00%> (ø)
...es/smooth_app/lib/data_models/fetched_product.dart 33.33% <33.33%> (+33.33%) ⬆️
packages/smooth_app/lib/main.dart 13.60% <100.00%> (+0.69%) ⬆️

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

You can test this PR on: Android

Impacted files:
* `fetched_product.dart`: added field `failedPingedHost` where we store the host that we couldn't ping
* `generated_plugin_registrant.cc`: wtf
* `generated_plugins.cmake`: wtf
* `GeneratedPluginRegistrant.swift`: wtf
* `main.dart`: registered `DartPingIOS`
* `product_refresher.dart`: now trying to ping the server if exception and connection
* `pubspec.lock`: wtf
* `pubspec.yaml: added packages `dart_ping` and `dart_ping_ios`
@github-actions github-actions bot added the MacOS label Aug 6, 2023
@monsieurtanuki
Copy link
Contributor Author

Additional possibility: "server down".

Let me sum up what may happen when we refresh a product:

  • we find the product => no error dialog
  • we don't find the product => "No product found" dialog
  • there was an exception
    • if there was no connectivity => "You're not connected to internet!" dialog
    • if we cannot ping the server => "Server down!"
    • final possibility => "Server error ($exception)"

Screenshot_2023-08-06-12-22-52

@teolemon
Copy link
Member

teolemon commented Aug 8, 2023

Analyzing smooth-app...

info • Sort directive sections alphabetically • packages/smooth_app/lib/pages/preferences/user_preferences_dev_mode.dart:10:1 • directives_ordering

@monsieurtanuki
Copy link
Contributor Author

Analyzing smooth-app...

info • Sort directive sections alphabetically • packages/smooth_app/lib/pages/preferences/user_preferences_dev_mode.dart:10:1 • directives_ordering

@teolemon Done.

@g123k
Copy link
Collaborator

g123k commented Aug 14, 2023

Additional possibility: "server down".

Let me sum up what may happen when we refresh a product:

  • we find the product => no error dialog

  • we don't find the product => "No product found" dialog

  • there was an exception

    • if there was no connectivity => "You're not connected to internet!" dialog
    • if we cannot ping the server => "Server down!"
    • final possibility => "Server error ($exception)"

Screenshot_2023-08-06-12-22-52

I think that would be way better that way.
Just by curiosity: why didn't you translate the new strings?

@monsieurtanuki
Copy link
Contributor Author

I think that would be way better that way.

@g123k That's the way it is coded in the latest version of the PR, so feel free to approve.

Just by curiosity: why didn't you translate the new strings?

If I were in a world where I had only ideas that were immediately approved, I would directly add the translations.
I currently don't live in that world, so instead of polluting the translations with labels that will never be used or giving me useless additional work, I prefer to put hard-coded strings, and if the code is approved I localize the strings.
This is not like we all agreed beforehand on the different cases and their related labels, here it's a bit more experimental.

@g123k
Copy link
Collaborator

g123k commented Aug 16, 2023

I think that would be way better that way.

@g123k That's the way it is coded in the latest version of the PR, so feel free to approve.

Just by curiosity: why didn't you translate the new strings?

If I were in a world where I had only ideas that were immediately approved, I would directly add the translations. I currently don't live in that world, so instead of polluting the translations with labels that will never be used or giving me useless additional work, I prefer to put hard-coded strings, and if the code is approved I localize the strings. This is not like we all agreed beforehand on the different cases and their related labels, here it's a bit more experimental.

Ok, so I approve your PR, so it's OK to translate ;)

Impacted files:
* `app_en.arb`: 4 new labels when we couldn't retrieve a product (not found, no internet, server down, server error)
* `product_refresher.dart`: used the new labels
@monsieurtanuki
Copy link
Contributor Author

Thank you @g123k for your review!

Probably related to #4549.
In general, we should test the connection and ping the server each time the server is called and the request failed.

@monsieurtanuki monsieurtanuki merged commit f79bae6 into openfoodfacts:develop Aug 16, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Hunger Games 🌐 l10n MacOS Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants