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

New Prices API in Django make some tests fail #970

Closed
monsieurtanuki opened this issue Sep 5, 2024 · 5 comments · Fixed by #971
Closed

New Prices API in Django make some tests fail #970

monsieurtanuki opened this issue Sep 5, 2024 · 5 comments · Fixed by #971
Assignees

Comments

@monsieurtanuki
Copy link
Contributor

Description

cf. openfoodfacts/open-prices#355 by @raphodn
The Prices API has recently been refactored in Django.
There are side-effects (e.g. tests now failing), cf. https://github.com/openfoodfacts/openfoodfacts-dart/actions/runs/10724138009/job/29739089653
To be solved in off-dart or in Prices. Or both.

Expected behavior

The Prices refactoring should have been transparent from off-dart.
Working on this issue, we'll see how bad that is.
May already have an impact on Prices for Smoothie.

cc. @teolemon @raphael0202

@monsieurtanuki monsieurtanuki self-assigned this Sep 5, 2024
@raphodn
Copy link
Member

raphodn commented Sep 5, 2024

started a PR here : openfoodfacts/open-prices#426

For now fixes

  • the 2 HTML returns (which should return JSON indeed) (line 142, 178)

To look into

  • the 401 vs 403 (line 123, 202)
    • edit : had a look, unlikely that this will be fixed, would require quite a bit of thought and work because of our custom authentication (session or cookie) : DRF docs
  • false vs true (line 117, 208)
    • need more context on this one

Won't "fix"

  • the ones where plain text was expected instead of json (line 106)
  • the ones where the error wording has changed (I'm sticking with default Django/DRF behavior) (line 131, 165)

@monsieurtanuki
Copy link
Contributor Author

Thank you @raphodn for your first glance!
I'll give you more details about the cryptic problems (e.g. the "true / false expected")

@monsieurtanuki
Copy link
Contributor Author

@raphodn The recent changes seem to have a limited impact on Smoothie.
The only one I detected was that the "top contributors" page doesn't work anymore.
That said, it looks like you're still developing live while I'm checking the errors, which makes my analysis less easy (for instance, 'Authentication credentials were not provided.' instead of 'Invalid authentication credentials') (and in that case my version is more accurate)

Following is my analysis.

  • line 106 (getAuthenticationToken)
    Probably no impact on Smoothie
    Now we receive something like {"error":"Invalid authentication credentials"}
    => Shouldn't we receive detail instead of error, something like {"detail":"Invalid authentication credentials"}?

  • line 117 (deleteUserSession): expected successful response status for "delete session" was 200, is now 204
    Probably no impact on Smoothie as we purposely ignored this response as we didn't want the task to fail just for not being able to delete the user session at the end.
    => Should we now expect a 204 status code for a successful "delete user session"?

  • line 123 (createPrice): expected response status for "invalid bearer token" was 401, is now 403*
    Probably no impact on Smoothie as we just check if it failed, we don't check the status code.
    => Should we now expect a 403 status code for a failed "create price" because the bearer token is invalid?

  • line 131 (getLocation): expected response message for "no location found" was something like Location with id -1 not found, is now No Location matches the given query.
    We don't use it in Smoothie anyway

  • line 142 (getOSMLocation): received unrelated HTML when failing
    We don't use it in Smoothie anyway

  • line 165 (getPriceProductById): : expected response message for "no product found" was something like Product with id -1 not found, is now No Location matches the given query.
    We don't use it in Smoothie anyway

  • line 178 (getPriceProductByCode): received unrelated HTML when failing
    We don't use it in Smoothie anyway

  • line 202 (uploadProof): expected response status for "invalid bearer token" was 401, is now 403
    Probably no impact on Smoothie as we just check if it failed, we don't check the status code.
    => Should we now expect a 403 status code for a failed "upload proof" because the bearer token is invalid?

  • line 208 (getUsers): we expect a bool is_moderator field for users, which is not there anymore.
    => should we consider is_moderator an optional field, or should you populate it? Maybe both.
    => impact on Smoothie: the "top price users" page doesn't work anymore - actually it still does, but in a funny way ;)

@raphodn
Copy link
Member

raphodn commented Sep 6, 2024

Ok about to merged the PR, it will fix :

  • the HTML returns (replaced with JSON) (line 142, 178)
  • replace {"error":"Invalid authentication credentials"} with {"detail":"Invalid authentication credentials"}
  • on 404, the message should now always be "No Model matches the given query."

Need to look into :

  • the User.is_moderator field not returned (always False for now as we don't use it yet)

In the wontfix :

  • for the delete actions, Django now correctly returns 204 instead of 200.
  • for the "invalid bearer token", we'll stick with the 403 for now

@monsieurtanuki
Copy link
Contributor Author

Fixed by #971.

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

Successfully merging a pull request may close this issue.

2 participants