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: reload product to fetch the language text #4717

Closed
wants to merge 3 commits into from

Conversation

LuanRoger
Copy link
Contributor

What

The product text is fetch from the server passing the current user language as parameter to it, so the server can return the appropriate text based on language. But the product is saved on a local database and the app get the product just if it doesn't exist.

This cause that the product text saved on the database may be on some other language, to avoid this to happen I redirect the user to PRODUCT_LOADER on packages/smooth_app/lib/cards/product_cards/smooth_product_card_found.dart (used on List tab), so every time that the user enter on product page it fetch from server (if possible).

Remark

If the user do a manual refresh (via RefreshIndicator) the text is fixed.

Fixes bug(s)

@LuanRoger LuanRoger requested a review from a team as a code owner October 22, 2023 13:06
@g123k
Copy link
Collaborator

g123k commented Oct 24, 2023

Hi @LuanRoger!

The problem with your solution is that the product on the carousel is still in the previous language.
I think you should instead implement a solution that forces a refresh of all/recent products once the language is changed

@LuanRoger
Copy link
Contributor Author

Every time that the user update the language it will get ALL the products on the local database and update them using the ProductRefresher.
Is not hard to get only the recenty added products to be updated, but for now I think it's a good solution.

Does look good, @g123k?

@M123-dev M123-dev requested a review from g123k October 30, 2023 16:04
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.

Hi @LuanRoger!

I guess your code would work, but with very problematic side-effects:

  • you load the whole product database in memory, and that would probably crash the app and/or be very slow (especially with the "offline: pre-download top 10K products"). A solution would be to load only the barcodes.
  • each time the user changes the language, you synchronously download all the relevant products from the server. Which means that it does not work without connection, with a low connection and even with a normal connection (as it will take a while with the user waiting on a frozen drop-down selector UI). A solution would be to use background tasks instead.

@LuanRoger
Copy link
Contributor Author

LuanRoger commented Jan 3, 2024

Hello @monsieurtanuki

Considering what you said on #4941 too, here my thoughts for possible solutions:

Smart fetch

The local database stores every product the user saw, so those are not important to fetch at first or even never, since the local DB also stores the products that the user find thrugh the search for example, which many of the users never will look for them again.

So instead of refresh all the products (considering the possible feature of slow bulks), refresh the ones in the user's created lists. For those is not in any list, the user continue to have the the manual refresh (RefreshIndicator on the product page).

Refresh last accesed products

I'm not familiar with the code base yet, I does't found any information about the last products accessed by the user on DaoProduct, a related one could be last_update column but does't seem to be the last access exactaly, but for test purpuses, I've created a simple method to get the firts n products ordening by last_update on decresent order:

Future<Map<String, Product>> getLast(int lasts) async {
    final Map<String, Product> result = <String, Product>{};
    
    final List<Map<String, dynamic>> queryResults =
          await localDatabase.database.query(
        _TABLE_PRODUCT,
        columns: _columns,
        orderBy: '$_TABLE_PRODUCT_COLUMN_LAST_UPDATE DESC',
        limit: lasts,
      );
      for (final Map<String, dynamic> row in queryResults) {
        result[row[_TABLE_PRODUCT_COLUMN_BARCODE] as String] =
            _getProductFromQueryResult(row);
      }
    
    return result;
}

But as I said, the last_update column seems not hold an access related information.

In offline

If the user change the language (or try to refresh the products in other way) while offline? Well, a possible solution could a kind flag, if the user try to refresh some product, the flag become true, so when the user back online, the app refreshes the products before enter in the home screen for example.

Others considerations

you load the whole product database in memory, and that would probably crash the app and/or be very slow (especially with the "offline: pre-download top 10K products"). A solution would be to load only the barcodes.

I didn't realize that 😅, I will fix that as soon as possible.

A solution would be to use background tasks instead.

Yes! I would consider that as a UX improvement, becaouse bofore implement that, is important to know whats need to be refreshed at first place, I think that, in the end, refresh all the products from the local database will not cause any major benefit.

What you think?

@monsieurtanuki
Copy link
Contributor

Smart fetch

Indeed we have to prioritize which products to refresh. The most important are the products in the history, then the user lists, then the rest.

Refresh last accessed products

Indeed last_update is not good enough, and that's why I created #4947.
Please also consider that last_update will be automatically refreshed by... erm... the refresh, so if we want to use it we'll have to compute a list of barcodes first (e.g. the products is user lists that have a last_update older than X), and then download them.
Btw your code for the getLast method would probably work but we would be slightly better off selecting just the barcode column and returning a List<String>.
A last_accessed column would be helpful.

In offline

Others considerations

The background tasks are not just a UX improvement, it's the only solution. The user cannot be stuck changing the language just because the connection is poor.

What you think?

I'll PR this week about adding the language metadata ("the last time this product was downloaded, it was in Italian").

With this additional metadata, we can do things lazily:

  • if the user changes the language, we create a new "refresh the language" background task
  • this task will select which local products to refresh (only those with a different language, possibly ordered by user list or last_update), but only the top 10.
  • the task then refreshes those 10 products
  • and at the end, the task creates a similar background task ("refresh the top 10 products that are not in the current language")

Somehow, the step by step behavior would be similar to BackgroundTaskOffline, BackgroundTaskTopBarcodes and BackgroundTaskDownloadProducts.

That said:

@LuanRoger
Copy link
Contributor Author

Ok them, so I'll be looking for some other issue to work with from time. Hope that you guys can fix this as soon as possible.

It was a pleasure to help.

@LuanRoger LuanRoger closed this Jan 4, 2024
@monsieurtanuki
Copy link
Contributor

@LuanRoger Thank you for your contribution: I will steal some of your suggestions ;)

@LuanRoger LuanRoger deleted the reload-for-update branch January 5, 2024 11:56
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.

When we change the language of the app, the product is not refreshed with the new language
4 participants