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

feature/#349 - history and scan lists now have displayed timestamps #401

Merged
merged 4 commits into from
Jun 17, 2021

Conversation

monsieurtanuki
Copy link
Contributor

History and Scan lists now behave differently from the other lists.
They use "extra data": a product is in the History (or the Scan) list
if it has as an "extra data" the "last seen timestamp" (or the last scan timestamp).

Bad news: as it's a new concept in the app, we'll start from scratch for both lists.

Impacted files:

  • continuous_scan_model.dart: new way of adding a product to scanned products; refactoring
  • continuous_scan_page.dart: unrelated color fix
  • dao_product.dart: added "last seen" and "last scan" as extra data, with a direct access to products that have those extra data
  • dao_product_list.dart: history and scan lists are now handled as "different" lists, computed from the "last seen" and "last scan" extra data; refactored
  • product_list.dart: added extra int data; cleaner "same product list" test
  • product_list_dialog_helper.dart: cleaner "same product list" test
  • product_list_page.dart: added dates to the display
  • product_page.dart: simplified history management
  • smooth_product_card_found.dart: added an optional refresh parameter, after visiting the product page

… timestamps

History and Scan lists now behave differently from the other lists.
They use "extra data": a product is in the History (or the Scan) list
if it has as an "extra data" the "last seen timestamp" (or the last scan timestamp).

Bad news: as it's a new concept in the app, we'll start from scratch for both lists.

Impacted files:
* `continuous_scan_model.dart`: new way of adding a product to scanned products; refactoring
* `continuous_scan_page.dart`: unrelated color fix
* `dao_product.dart`: added "last seen" and "last scan" as extra data, with a direct access to products that have those extra data
* `dao_product_list.dart`: history and scan lists are now handled as "different" lists, computed from the "last seen" and "last scan" extra data; refactored
* `product_list.dart`: added extra `int` data; cleaner "same product list" test
* `product_list_dialog_helper.dart`: cleaner "same product list" test
* `product_list_page.dart`: added dates to the display
* `product_page.dart`: simplified history management
* `smooth_product_card_found.dart`: added an optional `refresh` parameter, after visiting the product page
@M123-dev
Copy link
Member

M123-dev commented Jun 5, 2021

Thanks for the pr @monsieurtanuki, I don't think have a proper understanding of the part of the code, so I can't follow exactly what is happening, but there is nothing that catches my eye. I'll trust you on that.

Bad news: as it's a new concept in the app, we'll start from scratch for both lists.

What exactly has changed now, if I remember correctly we had only the normal lists in a db the rest in shared preferences. Has something changed in the db or in the anyway temporary shared prefs code?
What exactly do you have planned, how do you envision the new lists? Is there still the plan with several list types, since we had deactivated them for the time being.

@monsieurtanuki
Copy link
Contributor Author

Hi @M123-dev
For the moment I only changed the history and scan lists, not the user lists.

Regarding the database, we store single products in the product table, with the barcode as the primary key.
And we used to store history and scan lists in the product_list table: basically, a list of barcodes.
But we didn't store the products last seen or last scan timestamps, which are needed for #349.

In this PR, we don't use product_list for history or scan list anymore - though perhaps we could use product_list in order to store legacy data (with fake "now" timestamps), in order not to start from scratch.

The idea here is to say: we don't maintain explicit product lists for history and scan, let's use the additional product_extra table (where for a barcode and a "key" we store a "value").
We "build" both lists on the fly saying something like: "the history list is the ordered list of products that have a value in product_extra for the key "last_seen" - this stored value being the last seen timestamp for this product.

Doing so:

  • we have the needed additional data (last seen / last scan timestamp)
  • it's faster and easier to maintain lists - instead of storing the whole history list every time we see a product, we just say "for this barcode, let's store "now()" as the "last_seen" value

TL;DR
For the moment it only affects history and scan lists.
But I guess that could be generalized to all lists, and even to pantries and shopping lists that aren't currently in the database (but are in the shared preferences). In another issue / PR.

@monsieurtanuki
Copy link
Contributor Author

@M123-dev Actually I'll probably update this PR today, preparing the refactoring that will later include shopping lists and pantries.

@monsieurtanuki
Copy link
Contributor Author

@M123-dev @stephanegigandet @teolemon The main question that blocks me: does that make sense to see a product several times in the same list?

@teolemon
Copy link
Member

teolemon commented Jun 6, 2021

Hey, catching up, sorry.

  • On a User side: Displaying it several times does not make sense very often, and only in certain use cases, with additional features (like in food logging). In a simple history, no, it does not make much sense.
  • Technically speaking, logging all the times it was scanned, and using .last() would make sense, so that we don't have to change the DB schema in the future ?
  • Caching or not caching the history is also tricky, since pictures and product names, and scores might change over time.

@M123-dev
Copy link
Member

M123-dev commented Jun 6, 2021

does that make sense to see a product several times in the same list?

@monsieurtanuki I would say it depends on what kind of list. In pantries and normal lists not really, but in the history I would say it definitely makes sense.

When we look at Google Maps as example, when I look at a place there is sometimes a field "Last visited: xx.xx.xxxx". But you can also go and and visit the whole history, there double entries make sense in any case.

We have to choose what is our goal with the history. At least for me I am not that interested when I last saw a product in a store (and scanned it) but more what the name of the product I scanned last week was.

@monsieurtanuki
Copy link
Contributor Author

@teolemon @M123-dev Thank you guys for your answers.

Basically you say that it's needed to be able to store the same food several times in the same list - I'm lucky I asked because that was not my point of view at all.
I'm going to implement this. In first approach, I'll display all the occurrences of a product for a given list, then we'll refine the display.

@teolemon By the way I'm not storing product data history at all, just one version of the product (the latest) for a given barcode. And pictures are not cached at all either.

@stephanegigandet
Copy link
Contributor

@monsieurtanuki : I think it would indeed make sense to use the same mechanisms (and the same code) for all types of lists, scan history, favorites, pantry, food intake tracking etc.

Regarding the discussion between having multiple entries for one product, or only one: both use cases are interesting. And for pantry, it's even clearer that we need both: it's very interesting to see the current inventory of the items we have, and it's also very interesting to look at the history to see how many products we purchased in a month etc.

An easy and generic solution is to have two tables:

  1. a transaction log table, with one entry each time a product is added / removed from a list, with a date of the transaction
  2. a current state table, with a unique entry for each product in the list, and just a few stats (number of items currently in the list, date of last transaction)

It's like a bank account: we have the balance, and also the history of all transactions.

@monsieurtanuki
Copy link
Contributor Author

Thank you @stephanegigandet for your comment. You're right, it's better if we find tables that fit many purposes.

I'm not a big fan of a transaction table, as I cannot visualize currently clear use cases (except for food maniacs).
And you add either consistency issues or performance issues. Probably both.
I'll skip that part for the moment.

My suggestions regarding tables:

  • product (same as now): the primary key is the barcode, the content is the product as json
  • product_list (same as now): one row per list, like its name, its parameters, its colors, and a generated id as primary key
  • product_list_item: one row per barcode in a list, with an integer column for ordering data (like a timestamp or a 1,2,3 order), and a string value that contains a custom json string (scan timestamps for scan, product page timestamps for history, quantities for shopping list, quantities and dates for pantries)

With the custom json string, we can do whatever we want.
With the integer column, we can quickly sort the items without having to load all the items in order to decode their json and order the items accordingly.

What do you guys think of that?

@M123-dev
Copy link
Member

M123-dev commented Jun 7, 2021

@monsieurtanuki I think the idea is quite good, especially that we then only have to write a list code. But I would recommend that we add a type parameter to product_list which specifies what type of list we are using.

What do you think about also adding a string with json data to every product in the product table. In which then the scan timestamps and product page timestamps will be stored, because they are universal and not per list

@stephanegigandet
Copy link
Contributor

For the transactions, a very important use case is to be able to see one's consumpting over time, e.g. month to month. How many products I have eaten? How many packagings I have generated? Is the average Nutri-Score of food products I buy going down or up? Am I eating less and less meat? etc.

Even when scanning products, it's very useful so that you can say things like "You last bought this item on June 20th 2020".

And that's just obvious use cases, being able to look up history certainly opens many more use cases.

Regarding performance issues: I'm not very worried. There won't be a lot of transactions, and even if someone has 100 000 transactions (that's 10 products added or removed every day for 3 years), that wouldn't be a big problem...

@monsieurtanuki
Copy link
Contributor Author

@M123-dev My answers:

  • there's already a type parameter for product lists
  • your idea of adding to products a json with the scan and seen timestamps does not make sense: we definitely need sortable data like an integer (e.g. latest timestamp). Without that, we have to load all the products in order to decode their json, then sort the decoded jsons. Not practical for the latest 5 products in the home page. In addition to that, there's no reason not to consider history as a list of products (a special one, I admit).

@stephanegigandet We do agree, the use cases for a transaction table is for food maniacs and have not even been remotely mentioned in issues so far. I don't reject the concept, but it's not a priority at all. And even the way you see things, we'll need both aggregate tables (like now) and transaction tables: whatever I'm coding for aggregate tables is not going in the wrong direction. Later we'll have to add transactions aside, in new tables.
You're not worried with database performances. I am. Especially on old smartphones, and for the home page. That's why I benchmarked json and SQL databases.

@M123-dev
Copy link
Member

M123-dev commented Jun 7, 2021

we definitely need sortable data like an integer (e.g. latest timestamp). Without that, we have to load all the products in order to decode their json, then sort the decoded jsons.

@monsieurtanuki makes sense, I wasn't thinking about the history list, but rather when a known product was last scanned/opened. But I completely lost sight of the history list there.


For the transactions, a very important use case is to be able to see one's consumpting over time, e.g. month to month. How many products I have eaten? How many packagings I have generated? Is the average Nutri-Score of food products I buy going down or up? Am I eating less and less meat? etc.

@stephanegigandet, we should also be able to get these data with the current proposal from @monsieurtanuki.

If we add tracking functionality later, it would probably be better to create a list for products bought and a list for products eaten, rather than looking at when they were removed from a list. Especially the eaten list will also need a parameter for how much of a product has been eaten.

@stephanegigandet
Copy link
Contributor

@monsieurtanuki

whatever I'm coding for aggregate tables is not going in the wrong direction. Later we'll have to add transactions aside, in new tables.

Agreed.

…y and scan timestamps

In table `product_extra` we added an `int` column:
* the primary keys are barcode and "extra key" (e.g. an id for each different list)
* there is an `int` value and a `String` value
* the `int` value is used for ordering data (timestamps or 1,2,3,...)
* the `String` value is used for storing more complex data as json (e.g. the list of timestamps for history)

The contents of table `product_extra` can be stored in flutter code with the new class `ProductExtra` (basically, `int` and `String`)

For the moment the history and scan timestamps of each product are available via a lousy button at the end of the page.

New file:
* `product_extra.dart`: Extra data attached to a Product when it belongs to a ProductList

Impacted files:
* `dao_product.dart`: added an `int` column to the product extra table; now we use new class `ProductExtra`; refactored
* `dao_product_list.dart`: now we use new class `ProductExtra`; refactored
* `local_database.dart`: upgraded the db version because of a new column; added helper method `timestampToDateTime`
* `product_list.dart`: now we use new class `ProductExtra`; refactored
* `product_list_page.dart`: now we use new class `ProductExtra`
* `product_page.dart`: added a sample button at the end of the page in order to display all history and scans related to the product
@monsieurtanuki
Copy link
Contributor Author

For the moment the history and scan timestamps of each product are available via a lousy button at the end of the page:
Screenshot_2021-06-09-15-22-23

Screenshot_2021-06-09-15-22-17

Any opinion on that button is welcomed!

For the moment, only the history and scan timestamps are involved.
The idea is to extend that idea of [barcode, list id] => [int value, json value] to all lists, including the pantries and the shopping lists. But in another PR.

@monsieurtanuki
Copy link
Contributor Author

I suggest that we also store the "refresh from web" timestamp for each product, in a similar way as history and scans.
With that we could say which products need to be refreshed, e.g. with a one-week-guaranteed-freshness policy.

If you're ok with that, I can add this feature (just the new historical data, not the refresh policy) and merge the code.

@M123-dev
Copy link
Member

@monsieurtanuki good idea, I don't see any point against the implementation of that.

…refresh timestamps

New files:
* `abstract_dao.dart`: DAO abstraction
* `dao_product_extra.dart`: DAO for Product Extra data

Impacted files:
* `barcode_product_query.dart`: automatically storing the product in the database
* `continuous_scan_model.dart`: refactored
* `dao_product.dart`: now extends new class `AbstractDao`; moved code to new class `DaoProductExtra`; refactored; optimized bulk actions
* `dao_product_list.dart`: now extends new class `AbstractDao`; now using new class `DaoProductExtra`; removed a SQLite foreign key for performance reasons; not storing products anymore (it's done somewhere else)
* `database_product_list_supplier.dart`: refactored because of `ProductListSupplier`
* `local_database.dart`: new database version; added `DaoProductExtra`
* `main.dart`: more verbose database crash message
* `product_dialog_helper.dart`: minor refactoring
* `product_list_supplier.dart`: refactored
* `product_page.dart`: now we display the access, the scan and the refresh timestamps in the lousy temporary button
* `product_query_model.dart`: removed the code regarding storing products in the database as it's now done in the `QueryProductListSupplier`
* `product_query_page.dart`: minor refactoring
* `product_query_page_helper.dart`: refactored because of `ProductListSupplier`
* `query_product_list_supplier.dart`: automatically storing the products in the database; refactored because of `ProductListSupplier`
@monsieurtanuki
Copy link
Contributor Author

I've just added the "server refresh" timestamp history.

I left the ugly "timestamps" button at the bottom of the product page, at least temporarily, in order to be able to check the different timestamp lists and to think about how to display those timestamps with more subtlety. Feel free guys to remove that button.

I don't think there are many developers here ready to check my SQL code; if you don't mind I'll merge this PR without explicit approval by the end of the week.

Then I'll go on with the database refactoring (#166), that will make the following things possible:

  • reordering product lists
  • shopping lists and pantries in database (and not lousy SharedPreferences)

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Thanks for this (very huge) PR @monsieurtanuki this data is very nice to have.
I have not read any line in detail but the main picture looks good. Feel free to merge.
Such button should be no problem for development purposes, how and where we display the data has to be discussed anyways.

…refresh timestamps

Impacted file:
* `continuous_scan_model.dart`: minor private variable renaming
@monsieurtanuki monsieurtanuki merged commit 95f9e20 into openfoodfacts:develop Jun 17, 2021
@monsieurtanuki
Copy link
Contributor Author

Thank you @M123-dev for your comments!

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

Successfully merging this pull request may close these issues.

4 participants