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

[14.0][FIX] shopinvader_membership subscribe #1207

Conversation

acsonefho
Copy link
Contributor

No description provided.

@sbidoul
Copy link
Member

sbidoul commented Dec 20, 2021

@acsonefho a little bit of explanation would not hurt ;)

So this fixes the following error:

RESTFULL call to url https://***/shopinvader/membership/226226/subscribe with method POST and params {'membership_product_id': 226226} raise the following error 500 Internal Server Error: subscribe() got an unexpected keyword argument 'membership_product_id'

@rousseldenis
Copy link
Contributor

@acsonefho What's the status of this ??

@acsonefho acsonefho force-pushed the 14.0-shopinvader_membership_subscribe branch from 7f6cead to f30e2dd Compare March 21, 2022 15:24
@acsonefho acsonefho requested a review from sbidoul March 21, 2022 15:25
@sbidoul
Copy link
Member

sbidoul commented Mar 21, 2022

LGTM. Test are red, though. Can you post a screenshot of the swagger ?

BTW, I take the opportunity to remind that these services exposing standard Odoo or OCA features that are not related to shopinvader don't need a dependency on shopinvader, they just need to depend on base_rest and use authenticated_partner_id in the service context. Which also mean they can go in OCA, like OCA/event#247 (event) and OCA/helpdesk#270 (helpdesk_mgmt).

@acsonefho acsonefho force-pushed the 14.0-shopinvader_membership_subscribe branch from f30e2dd to f29efdc Compare March 21, 2022 16:49
@acsonefho
Copy link
Contributor Author

LGTM. Test are red, though. Can you post a screenshot of the swagger ?

image

@sbidoul
Copy link
Member

sbidoul commented Mar 21, 2022

Do we need to keep the GET /{_id}/subscribe for backward compatibility ? If yes, can you please add a note in the docs saying it is deprecated. A GET must never have side effects.

@sbidoul
Copy link
Member

sbidoul commented Mar 21, 2022

And can you show a screenshot of the input form swagger is creating for the POST ? I'm not entirely fluent in cerberus :)

@acsonefho
Copy link
Contributor Author

And can you show a screenshot of the input form swagger is creating for the POST ? I'm not entirely fluent in cerberus :)

image

Do we need to keep the GET /{_id}/subscribe for backward compatibility ?

I update the documentation. I update the GET with a deprecated (+ also add a warning into logs when this GET is used).

@acsonefho acsonefho force-pushed the 14.0-shopinvader_membership_subscribe branch from f29efdc to b831ff8 Compare March 22, 2022 06:47
wizard = self.env["membership.invoice"].create(
{"product_id": _id, "member_price": membership_product.list_price}
{"product_id": product_id, "member_price": membership_product.list_price}
)
Copy link
Member

Choose a reason for hiding this comment

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

One last thing: I'd rather avoid using the wizard and call res.partner.create_membership_invoice() instead. The code will be easier to understand and the wizard will stay where it needs to stay (i.e. in the UI realm and not the business logic realm).

@rousseldenis
Copy link
Contributor

@acsonefho

@sebastienbeau sebastienbeau added this to the 14.0 milestone Jun 9, 2023
@sebastienbeau sebastienbeau added the change Change in existing module label Jun 14, 2023
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale label Mar 31, 2024
@github-actions github-actions bot closed this May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Change in existing module needs fixing stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants