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

[16.0][MIG] shopinvader_api_delivery_carrier: FastAPI services #1479

Conversation

marielejeune
Copy link
Contributor

@marielejeune marielejeune commented Nov 29, 2023

Migration of shopinvader_delivery_carrier to FastAPI services.

  1. shopinvader_delivery_carrier contains the Shopinvader logic related to delivery.
  2. shopinvader_api_delivery_carrier contains all FastAPI schemas and services:
    • A new delivery_carrier router, with a route to search on delivery.carrier, filtering by country and/or zipcode.
    • A new delivery router, with a route to search on all pickings related to the user.
    • A new set_carrier route under the cart router, to set the carrier on the cart.
    • An override of sync route under the cart router that removes the carrier on the cart everytime a new transaction is synchronized.

Features & changes
This is a migration to version 16 of shopinvader_delivery_carrier but combined with a rewriting to FastAPI, which means that some features are not integrated (yet?). Here is a summary of the differences:

  • There existed a configuration on shopinvader.backend to configure which delivery order states are visible. This configuration was used to filter the visible pickings on the front side.
    The notion of shopinvader backend is not kept in v16 and we don't need this configuration for now, so we don't preserve it here.
  • There was a service on deliveries to download the stock.action_report_delivery report. We didn't keep this feature for now.
  • In version 14 an anonymous user could search on deliveries, and an empty list was always returned.
    Here the route is authenticated so the partner will never be anonymous.
  • There were notifications sent to the backend when a picking was done. Notifications feature is not migrated in v16.

@marielejeune marielejeune force-pushed the 16.0-shopinvader_api_delivery_carrier-mle branch from 6d04350 to 90196b1 Compare November 29, 2023 12:48
@marielejeune marielejeune changed the title [WIP][16.0][MIG] shopinvader_api_delivery_carrier [WIP][16.0][MIG] shopinvader_api_delivery_carrier: FastAPI services Nov 29, 2023
@marielejeune marielejeune force-pushed the 16.0-shopinvader_api_delivery_carrier-mle branch 4 times, most recently from 4328f2f to 4175e9f Compare February 6, 2024 09:09
@marielejeune marielejeune force-pushed the 16.0-shopinvader_api_delivery_carrier-mle branch 3 times, most recently from c5eda37 to 97148ca Compare February 13, 2024 08:21
@marielejeune marielejeune changed the title [WIP][16.0][MIG] shopinvader_api_delivery_carrier: FastAPI services [16.0][MIG] shopinvader_api_delivery_carrier: FastAPI services Feb 13, 2024
@marielejeune marielejeune marked this pull request as ready for review February 13, 2024 08:21
@marielejeune marielejeune force-pushed the 16.0-shopinvader_api_delivery_carrier-mle branch from 97148ca to 41db857 Compare February 13, 2024 08:50
@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="utf-8" ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't RR go to their own module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done for principal modules, but not everywhere (for eg. RR are in shopinvader_api_sale_loyalty).
@lmignon what's your opinion about it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine to get it here. We try to get a proper security declaration by api.... This addon comes with its own security group. If a same alc is defined multiple time across different addon we could create a common addon but is-it really better to get one addon for one ACL?

cls.partner = cls.env["res.partner"].create(
{"name": "FastAPI Delivery Carrier Demo"}
)
cls.user_with_rights = cls.env["res.users"].create(
Copy link
Contributor

Choose a reason for hiding this comment

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

always set no_reset_password in ctx when creating users (or use new_test_user from odoo.tests.common)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (using new_test_user)



@delivery_carrier_router.get("/{uuid}/delivery_carriers")
@delivery_carrier_router.get("/current/delivery_carriers")
Copy link
Contributor

Choose a reason for hiding this comment

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

I keep seeing this current... what's the point of it? I know it comes from the cart endpoint but I don't get why we need a specific key to default to the last used cart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the default route @delivery_carrier_router.get("/delivery_carriers") is used to return all delivery carriers, not only the ones applicable to the current cart.

Copy link
Collaborator

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

LGTM (Code Review)
Thank you @marielejeune

@shopinvader-git-bot
Copy link

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sebastienbeau sebastienbeau added this to the 16.0 milestone Jun 3, 2024
@lmignon
Copy link
Collaborator

lmignon commented Jun 3, 2024

Modules moved to odoo-shopinvader-carrier shopinvader/odoo-shopinvader-carrier#8

@lmignon lmignon closed this Jun 3, 2024
@lmignon lmignon deleted the 16.0-shopinvader_api_delivery_carrier-mle branch June 3, 2024 16:43
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.

7 participants