-
-
Notifications
You must be signed in to change notification settings - Fork 176
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
Migrate account_fiscal_position_rule_sale to v10.0 #68
Migrate account_fiscal_position_rule_sale to v10.0 #68
Conversation
Hey @TimLai125, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
But this one depends on account_fiscal_position_rule. Where it is? |
@pedrobaeza account_fiscal_position_rule has been migrated and merged in #61, do you mean I need to make any further adjustments on the account_fiscal_position_rule? |
Oh, sorry, it was not written in the migration issue. |
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.
Code reviewed. LGTM. 👍
} | ||
# If the fiscal position of the customer is set, we respect the | ||
# setting. | ||
super(SaleOrder, self).onchange_partner_shipping_id() |
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 do not understand this part. The notes does not match woth what it then does. When do you really want to really avoid updating the fiscal position? I would say if the partner_shipping_id has changed, I would apply the new fiscal position.
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.
@jbeficent As I look at the code again, I agree that this part should be improved if we are to migrate this module.
What I just realized is that the standard function actually since V9 covers the particular requirement we were trying to cover using this module - automatically propose the correct fiscal position based on the destination state. Ref: https://github.com/odoo/odoo/blob/be7927d629b765c4f3eeb3ada11806ba75d909ca/addons/account/models/partner.py#L94-L134
The main function the standard Odoo may be missing is the ability to consider the address of the source location. Therefore, if we are to cover this missing feature, I guess we should probably review the overall design of the modules in this repo as the large part of the functions these modules are trying to deliver are redundant with what standard Odoo now provides. I guess just extending the fiscal position model (instead of having fiscal position rule model) would make more sense.
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 need to revoke my earlier approval. ^^;
} | ||
# If the fiscal position of the customer is set, we respect the | ||
# setting. | ||
super(SaleOrder, self).onchange_partner_shipping_id() |
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.
@jbeficent As I look at the code again, I agree that this part should be improved if we are to migrate this module.
What I just realized is that the standard function actually since V9 covers the particular requirement we were trying to cover using this module - automatically propose the correct fiscal position based on the destination state. Ref: https://github.com/odoo/odoo/blob/be7927d629b765c4f3eeb3ada11806ba75d909ca/addons/account/models/partner.py#L94-L134
The main function the standard Odoo may be missing is the ability to consider the address of the source location. Therefore, if we are to cover this missing feature, I guess we should probably review the overall design of the modules in this repo as the large part of the functions these modules are trying to deliver are redundant with what standard Odoo now provides. I guess just extending the fiscal position model (instead of having fiscal position rule model) would make more sense.
cc @renatonlima I think you were working on that one too, may be you can check |
According the comments from @yostashiro , most of the functionalities of this module are covered by standard. |
Hey @TimLai125, Appreciation of efforts, |
No description provided.