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: Add status response model for POST/PUT/DELETE endpoints #239

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

suchithh
Copy link
Contributor

@suchithh suchithh commented Mar 4, 2025

What

This change modifies the API. As per recommendations in thread #231 (comment), #231 was split into this PR which changes the API.

Changes

  • Added a status response model to convert simple "ok" returns on success of POST, PUT, and DELETE endpoints into a proper Pydantic model

Related Issues

Partially fixes concern raised in #119 regarding having multiple return types. #231 (comment) is my reasoning behind why we should have standardization within each endpoint and across the API.

@suchithh suchithh requested a review from a team as a code owner March 4, 2025 23:31
@suchithh suchithh changed the title Add status response model for POST/PUT/DELETE endpoints fix: Add status response model for POST/PUT/DELETE endpoints Mar 4, 2025
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.59%. Comparing base (b9cda65) to head (f4866c1).
Report is 68 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
- Coverage   95.06%   94.59%   -0.47%     
==========================================
  Files           5        5              
  Lines         324      370      +46     
==========================================
+ Hits          308      350      +42     
- Misses         16       20       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alexgarel
Copy link
Member

@suchithh I think we can't change the API without consulting the mobile app team (it's easy to change expected response in openfoodfacts-server, but it's harder on a mobile app, there is a risk of crash).

Can you open an issue in https://github.com/openfoodfacts/smooth-app/ to ask if it won't crash the mobile app if we change this ? (please detail you issue to help them answer quickly).

After that we can open an issue for openfoodfacts-server and openfoodfacts-explorer to leave them the time to change the code to handle the new API before merging this one.

@alexgarel
Copy link
Member

@suchithh you might look for yourself in the mobile app code (or openfoodfacts-dart which is supporting it), because they might just check the returned http status, in which case it's ok to change the API.

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

Successfully merging this pull request may close these issues.

3 participants