-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
8.0 ref port account product fiscal classification #28
8.0 ref port account product fiscal classification #28
Conversation
all the taxes; | ||
|
||
.. image:: ./static/src/img/product_template_accounting_setting.png | ||
|
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.
These images are not loading correctly. The names and path are wrong.
@danimaribeiro : about that comment, #24 (comment) i didn't create a new description field, because the new name has big size. (256), the old one was 32 and the old description was 64. |
product_tmpl_qty = fields.Integer( | ||
string='Products Quantity', compute='_compute_product_tmpl_info') | ||
|
||
purchase_tax_ids = fields.Many2many( |
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.
FYI : about #24 (comment)
I set purchase_tax_ids instead of purchase_base_tax_ids, for clarity. I used oldname and I set the same table name, so upgrade should work.
If your custom modules overload this field and sale_base_tax_ids, please make the changes.
@ ALL : about the migration between a database V7 using account_product_fiscal_classification (with properties) and a V8 DB using this account_product_fiscal_classification module, I suggest you to simply uninstall the module before the migration. The installation of the module in V8 will create again all fiscal classification depending of all your product template configuration and set them to the according templates. |
doc = etree.XML(res['arch']) | ||
nodes = doc.xpath("//field[@name='fiscal_classification_id']") | ||
if nodes: | ||
nodes[0].set('required', '1') |
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.
I just didn't understand why you have to override fields_view_get just to set required.
Why not put to xml view directly?
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.
Hi @danimaribeiro,
Indeed, my explication was not clear. I updated it. Please let me know if you have any doubt.
https://github.com/OCA/account-fiscal-rule/pull/28/files#diff-95ba4f4a046cd577c87a220edad46e0dR59
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.
Ok thanks.
About the description there is no problem if you don't create, we inherit and add to it.
We use the name for the code, and description to description of classification, maybe we create the code instead of descritption.
For me it's ok now, I also asked @mileo to test this.
I did a full test in all functionalities. |
hey thanks @legalsylvain I was about to ping you about that one again. Seems you need some nice work, I'll review that in detail tomorrow. |
@danimaribeiro or @mileo : could you pull translation again (BR) ? |
I do the translations. |
} | ||
super(ProductTemplate, self.sudo()).write(tax_vals) | ||
elif 'supplier_taxes_id' in vals.keys() or 'taxes_id' in vals.keys(): | ||
# product template Single update mode |
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.
I can't see how this branch of code will be used if the by normal user if in view this fields are readonly
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.
- all cases : While importing data. (see description of the module);
https://github.com/OCA/account-fiscal-rule/pull/28/files#diff-39aa8614d5d8e95ba38834e7370a8b07R34 - custom cases : if a custom module set taxes not readonly; (to all user or some specific like accountant users)
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.
💯
LGTM
Tested, nice work, we can do nice features in l10n-brazil with your work. LGTM, just #28 (comment) I can help in translations too. |
@danimaribeiro, @mileo : pot file added. I set also a field as translate = true. |
I guys, Maybe we can use transfix to manage translation for this repo? OCA/maintainer-quality-tools#194 I'll review that in detail... thanks @legalsylvain |
@legalsylvain I sent you a PR with translations. @renatonlima The translations in transifex work just after merge. |
Thanks. Merged. |
@rvalyi , @renatonlima : any news ? |
Hi @legalsylvain, Sorry for delay, LGTM! Some remarks:
I think we can aprove this PR and create others PR to implement these remarks Good work! kind regards. |
@renatonlima : Thanks for your review. @ALL : this PR can be merged. (3 approvals) |
@legalsylvain could you rebase on 8.0 please? |
…nly called at the installation, and no during update anymore;
5743141
to
0c7dafa
Compare
@rvalyi : done. |
…classification 8.0 ref port account product fiscal classification
This PR :
@danimaribeiro : sorry but I dropped your brazilian translation because all fields and model have changed.