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] Add shopinvader_schema_invoice + shopinvader_api_invoice + shopinvader_api_security_invoice #1493

Open
wants to merge 3 commits into
base: 16.0
Choose a base branch
from

Conversation

simahawk
Copy link
Contributor

@simahawk simahawk commented Jan 9, 2024

Replaces #1365

Comment on lines +19 to +35
ref: str | str
payment_reference: str | str
Copy link
Member

Choose a reason for hiding this comment

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

Why not

Suggested change
ref: str | str
payment_reference: str | str
ref: str | None
payment_reference: str | None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is normally I string I'd rather stick to an empty string as default.
Do you see any implications?

Copy link
Member

Choose a reason for hiding this comment

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

It may be a matter of test. str | None perhaps communicates more explicitly that the value can be set or not set.

Copy link
Collaborator

Choose a reason for hiding this comment

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

str | None means the value can be a string or None. I don't understant what str | str means. If you want an empty string by default it should be declared as 'ref : str = ""'

@sbidoul
Copy link
Member

sbidoul commented Jan 10, 2024

Thanks for doing this, @simahawk !

Copy link
Contributor

@marielejeune marielejeune left a comment

Choose a reason for hiding this comment

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

LGTM (nothing more than Stephane's comments). Thanks for having done it!

@simahawk simahawk changed the title [16.0] Add shopinvader_schema_invoice + shopinvader_api_invoice [16.0] Add shopinvader_schema_invoice + shopinvader_api_invoice + shopinvader_api_security_invoice Feb 29, 2024
@simahawk
Copy link
Contributor Author

@sbidoul @lmignon would you mind to pass by again when you have time?
I followed up w/ your comments + added download endpoint + security.

@simahawk simahawk removed the WIP label Feb 29, 2024
It introduces the following schema:

* Invoice
* InvoiceAmount
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be updated

# Moreover, having a rule for records not tied to partners
# can be quite complicated.
# Let's use sudo!
count = env["account.move"].sudo().search_count(domain)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could have a RR that give access to invoice to the authenticated partner. In this way you can search without sudo and then call Invoice.from_records(invoices.sudo()) We are therefore sure that the security is properly applied.

Comment on lines +19 to +35
ref: str | str
payment_reference: str | str
Copy link
Collaborator

Choose a reason for hiding this comment

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

str | None means the value can be a string or None. I don't understant what str | str means. If you want an empty string by default it should be declared as 'ref : str = ""'

amount: InvoiceAmount | None

@classmethod
def from_record(cls, odoo_rec):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to name from which odoo model we construct the fastapi model.

Suggested change
def from_record(cls, odoo_rec):
def from_account_move(cls, odoo_rec):

)

@classmethod
def from_records(cls, odoo_recs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

see previous comment

amount_due: float | None

@classmethod
def from_record(cls, odoo_rec):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feom which odoo model?

Suggested change
def from_record(cls, odoo_rec):
def from_account_move(cls, odoo_rec):

from odoo.tools.float_utils import float_round


class InvoiceAmount(StrictExtendableBaseModel, extra="ignore"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need to declare extra="ignore"? A common use of this parameter is for models used as incoming message. In this case we want to ignore unknown attributes.

invoicing_legacy = "invoicing_legacy"


class Invoice(StrictExtendableBaseModel, extra="ignore"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

? extra="ignore"?

@paradoxxxzero
Copy link
Contributor

paradoxxxzero commented Dec 3, 2024

Superseded by #1572

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.

6 participants