-
Notifications
You must be signed in to change notification settings - Fork 116
18.0 rd accounting onboarding jugac #4862
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
18.0 rd accounting onboarding jugac #4862
Conversation
|
This PR targets the un-managed branch odoo-dev/odoo:18.0-rd-accounting-onboarding-malb, it needs to be retargeted before it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have a lot of commit i will do one by one
--> For the quick search:
Commit message title is good but there is no description of the change you did, for bigger pr it will be impossible to understand what is being done, so always add a description of the change 😄
you can follow that guideline:
https://www.odoo.com/documentation/17.0/contributing/development/git_guidelines.html#commit-message-structure
| <field name="name" string="Journal Item" filter_domain="[ | ||
| '|', '|', '|', | ||
| ('name', 'ilike', self), ('ref', 'ilike', self), ('account_id', 'ilike', self), ('partner_id', 'ilike', self)]"/> | ||
| <field name="amount_currency" filter_domain="[('amount_currency', '>=', self)]" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You used the amount_currency but i don't think that really what we want 👀 The amount currency will be the same as the balance or debit and credit when we have the same currency but it could happen that you do a transaction in dollars when your company is in euro and so the amount_currency will be different. Here we want to be able to search on the balance or debit and credit together 😄
also amount currency could be negative then your code will not work 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make due date invisible for cancelled invoices --> Change is looking good ! nice work on this but still commit message could be better hehe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--> Remove points of menu about analytics if analytics ...
Solution proposed is the right one good job ! 😄 Let's elaborate a bit more the commit message though and then it will be perfect hehe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Group payment -->
Commit message will need a bit more explanation and also let's try to fix the tests 👀
| 'use_date_range': True, | ||
| 'company_id': self.id, | ||
| 'prefix': 'BATCH/%(year)s/', | ||
| 'prefix': 'PAY/%(year)s/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is good but i think that if runbot was working like a normal pr, you would have a lot of test failing 👀 Take a look at addons/account/tests/test_account_payment_register.py 😄 Run the tests and check the ones that fails and fix them 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delivery date -->
Commit message as usual and few change to be done 😄
| # EXTENDS 'account' | ||
| super()._compute_show_delivery_date() | ||
| for move in self: | ||
| if move.country_code == 'FR': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use l10n_fr_is_company_french instead 👀 Check why and tell me 😄
| for move in self: | ||
| if move.country_code == 'FR' and move.is_sale_document() and not move.delivery_date: | ||
| move.delivery_date = move.invoice_date or fields.Date.context_today(self) | ||
| return super()._post(soft) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general we put the super on top of the function, because it would happens that the delivery date is modified else where and so the change will not work, also always add the comment for the extend when doing an extension 😄
|
|
||
| def _post(self, soft=True): | ||
| for move in self: | ||
| if move.country_code == 'FR' and move.is_sale_document() and not move.delivery_date: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use show_delivery_date 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add more visibility about the sent status -->
Commit message needs to be changed and few nitpicking 😄
| invoice_sent_status = fields.Selection( | ||
| selection=[('sent', 'Sent'), ('not_sent', 'Not sent')], | ||
| compute="_compute_is_sent", | ||
| copy=False | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule of thumbs in python, use double quotes when it's something the user will see otherwise use single quotes
| invoice_sent_status = fields.Selection( | |
| selection=[('sent', 'Sent'), ('not_sent', 'Not sent')], | |
| compute="_compute_is_sent", | |
| copy=False | |
| ) | |
| invoice_sent_status = fields.Selection( | |
| selection=[ | |
| ('sent', "Sent"), | |
| ('not_sent', "Not sent"), | |
| ], | |
| compute='_compute_is_sent', | |
| copy=False | |
| ) |
| invisible="payment_state == 'invoicing_legacy' or move_type == 'entry'" | ||
| optional="show" | ||
| /> | ||
| <field name="invoice_sent_status" string="Sent" optional="hide" widget="badge" decoration-success="is_move_sent" decoration-warning="not is_move_sent"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be nice to have a filter for the invoice sent, look at <filter name="not_sent" 👀
No description provided.