openfoodfacts / openfoodfacts-dart

Open Food Facts API Wrapper
https://pub.dev/packages/openfoodfacts
Apache License 2.0
167 stars 67 forks source link

Price proof: link a date, location & currency #942

Closed monsieurtanuki closed 4 months ago

monsieurtanuki commented 5 months ago
          @monsieurtanuki in the next few days i will focus on integrating these new Proof fields.

Originally posted by @raphodn in https://github.com/openfoodfacts/open-prices-frontend/issues/634#issuecomment-2178706438

monsieurtanuki commented 4 months ago

@raphodn I've just tried to update a proof, and I got the following error message. Any idea what's wrong with my call?

{
  "detail":[
    {
      "type":"extra_forbidden",
      "loc":["body","app_name"],
      "msg":"Extra inputs are not permitted",
      "input":"off-dart integration tests",
      "url":"https://errors.pydantic.dev/2.6/v/extra_forbidden"
    }
  ]
}
raphodn commented 4 months ago

I've just tried to update a proof, and I got the following error message. Any idea what's wrong with my call?

From what i see the server expects a strict list of items in the body.

monsieurtanuki commented 4 months ago

Thank you @raphodn for your answer, now it's solved!

raphodn commented 4 months ago

What's still missing is to be able to update the proof's location. I need some thoughts on this first...

monsieurtanuki commented 4 months ago

What's still missing is to be able to update the proof's location. I need some thoughts on this first...

I don't see the problem at all, actually. It's just some other fields. You mean UX-wise?

raphodn commented 4 months ago

Also in terms of fields. We currently ask for a location_osm_id and location_osm_type, and then link it to an actual location object (and set the location_id.

Not sure if this is maintainable in the long run. I'll see :)

monsieurtanuki commented 4 months ago

Anyway you already have to deal with incoherences, e.g.:

raphodn commented 4 months ago

True, but the incoherences would only come from non-web-or-mobile users (that use the API directly). And for them (and everyone else) we could start enforcing data quality rules : https://github.com/openfoodfacts/open-prices/discussions/319

monsieurtanuki commented 4 months ago

Indeed.

Btw I've just tried to create a price with a value for price_per (never tried before), and the value doesn't seem to be set (it is null). It works with updatePrice, though. Could you have a look at it?

raphodn commented 4 months ago

What value did you try for price_per ? the 2 valid options are UNIT or KILOGRAM. https://github.com/openfoodfacts/open-prices/blob/6d3c84ddf10116a98ad6db146a3c51eee60ad89f/app/enums.py#L26 And there's also a validator that sets this value to null if the price has a product (as its only for non-barcode prices). https://github.com/openfoodfacts/open-prices/blob/6d3c84ddf10116a98ad6db146a3c51eee60ad89f/app/schemas.py#L417

monsieurtanuki commented 4 months ago

@raphodn Understood now that price_per is only for non-barcodes, thanks! Then, there's a little bug in your update checks, as I managed to update a price_per for a barcode product.

raphodn commented 4 months ago

Oh yeah I see the problem, we only do the validation on create, not update :facepalm:

raphodn commented 4 months ago

ok @monsieurtanuki I did some quick changes in the backend here : https://github.com/openfoodfacts/open-prices/pull/342 & https://github.com/openfoodfacts/open-prices/pull/343

Doing some fixes in the frontend to reflect this : https://github.com/openfoodfacts/open-prices-frontend/pull/668

I merged so its available in staging, i'll wait for your go to deploy in prod (to be sure that it doesn't break your current price/proof addition in the mobile app)

monsieurtanuki commented 4 months ago

@raphodn I've just re-run our 17 off-dart tests about prices, and they all pass. All the "write" tests work on TEST .net env. As long as you've added optional parameters for existing methods, that should be fine.

raphodn commented 4 months ago

Ok awesome, I'll deploy then, so that if there is any issue I can fix it tonight or tomorrow :pray:

monsieurtanuki commented 4 months ago

@raphodn LGTM: I've just added a receipt today with Smoothie (without changing anything in the code) https://prices.openfoodfacts.org/app/users/monsieurtanuki

raphodn commented 4 months ago

On the "location update" subject I just created an issue for a new endpoint to create a location, will work on it in the coming hours/days : https://github.com/openfoodfacts/open-prices/issues/346

monsieurtanuki commented 4 months ago

@raphodn I think you should systematically update the proof location/currency/date from the first price, if empty. I've just had at look at today's proof, and the "missing fields" effect doesn't look good. Screenshot_20240628_220925_Chrome

raphodn commented 4 months ago

Well the idea is that the location, date & currency of a proof should be mandatory. I didn't implement it that way (yet) because there are proofs without prices (so i wasn't able to init them with this data).

In the web app I made the changes so that the users have to provide these 3 info if you want to add a price afterwards (and there's indeed a bug/limitation if you choose an existing proof without these info).

Your screenshot is a proof uploaded from mobile i presume ?

Do you plan on implementing these new fields at the proof level ? It would be the best workflow don't you think ?

raphodn commented 4 months ago

These new fields are optional because some proofs (like GDPR) may have multiple locations & dates. But the idea is that they should be mandatory for PRICE_TAG & RECEIPT proofs..

Will implement these validation rules ASAP

monsieurtanuki commented 4 months ago

Your screenshot is a proof uploaded from mobile i presume ?

The screenshot was taken on a mobile, from the web app. The related data came from Smoothie, as the test I ran yesterday.

Do you plan on implementing these new fields at the proof level ?

First the code in dart has to be reviewed, approved and merged, then it will be easy to integrate in Smoothie/ flutter.

It would be the best workflow don't you think ?

Don't trust anyone who uses the API, by default. Like our discussion about you accepting images as .bin because the mime header was not set. The solution there was to add a mandatory mime type parameter, if I remember well.

These new fields are optional because some proofs (like GDPR) may have multiple locations & dates. But the idea is that they should be mandatory for PRICE_TAG & RECEIPT proofs..

Then I would recommend considering GDPR so different from receipt/price tags than they deserve a different API entry point.

Will implement these validation rules ASAP

Please don't! Smoothie wouldn't work anymore. Or do it as api/v2.

raphodn commented 4 months ago

Please don't! Smoothie wouldn't work anymore.

Then how should we proceed ? This is an evolution that was validated collectively in the weekly Wednesday meeting 10 days ago. And we pushed back the mobile launch (discussion on Slack) because we considered that these changes (having a proof location, date & currency) was necassary to increase data quality and simplifying user input. cc @teolemon

I'm going to do the PRs nevertheless, but won't merge them. edit : PR opened here : https://github.com/openfoodfacts/open-prices/pull/349

Fyi i checked the database

7481 proofs
461 without date
1 without date and souce is 'Smoothie - OpenFoodFacts'
monsieurtanuki commented 4 months ago

I wasn't aware of those decisions. I assumed that an api/v2 version would do the trick: letting "old" users work without errors with v1 (though deprecated) and letting up-to-date users setting better quality data. If we're supposed to be in such an early stage with limited number of apps (e.g. only web app and Smoothie) that we can reject new proofs if fields are missing because of "old" code, why not, fair enough. In the same spirit, we shouldn't populate currency for product prices as it is redundant (if we're lucky) or incoherent.

raphodn commented 4 months ago

It's early stage, there's no known API users apart from the web and now the mobile, so it will be under the same api/v1 :)

monsieurtanuki commented 4 months ago

@raphodn Fair enough, but the current Smoothie users (beta only?) won't be able to add prices while the code isn't changed / merged for Smoothie. And for the record the code in dart hasn't even been reviewed yet.

raphodn commented 4 months ago

As long as the price have a location/date/currency (which is the case in the mobile's current version) then we'll be able to retro-actively populate the linked proof location/date/currency fields (with a quick script).

monsieurtanuki commented 4 months ago

As long as the price have a location/date/currency (which is the case in the mobile's current version)

Perhaps it would be a good idea to keep those parameters anyway at the price level (what would a price of 1 mean?) and to use them to check the consistency with the proof (and reject inconsistent proof and price parameters).

raphodn commented 4 months ago

Yes for now there's no plan to remove the price location/date/currency fields 😇