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 mig l10n fr ecotaxe #448

Merged
merged 54 commits into from
Jun 5, 2024
Merged

Conversation

mourad-ehm
Copy link
Contributor

No description provided.

@legalsylvain legalsylvain added this to the 16.0 milestone Jun 9, 2023
@legalsylvain
Copy link
Contributor

/ocabot migration l10n_fr_ecotaxe

@OCA-git-bot OCA-git-bot mentioned this pull request Jun 9, 2023
29 tasks
@pfranck
Copy link

pfranck commented Jun 29, 2023

Any help needed to merge the request ?

@pfranck
Copy link

pfranck commented Sep 6, 2023

Any solution to be able to merge this request

Copy link
Contributor

@florian-dacosta florian-dacosta left a comment

Choose a reason for hiding this comment

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

I did a quick code review, just some minor remarks.
When the PR is ready I will do some tests

ecotaxe_scale_code = fields.Char()

@api.onchange("ecotaxe_type")
def _onchange_ecotaxe_type(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to convert default_fixed_ecotaxe and ecotaxe_coef as readonly=False computed fields to replace this on change.



class EcotaxeLineProduct(models.Model):
"""class for objects which can be used to save mutili ecotaxe calssification by product."""
Copy link
Contributor

Choose a reason for hiding this comment

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

mutili = multi

l10n_fr_ecotaxe/models/ecotaxe_line_product.py Outdated Show resolved Hide resolved
Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

Partial review

readonly="1"
>
<tree editable="bottom">
<field name="ecotaxe_classification_id" />
Copy link
Member

Choose a reason for hiding this comment

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

Just for usability

We already know it's Ecotax because this is the name of th o2m

Suggested change
<field name="ecotaxe_classification_id" />
<field name="ecotaxe_classification_id" string="Classification" />

<tree editable="bottom">
<field name="ecotaxe_classification_id" />
<field name="force_ecotaxe_amount" />
<field name="ecotaxe_amount" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<field name="ecotaxe_amount" />
<field name="ecotaxe_amount" string="Amount" />

<tree editable="bottom">
<field name="ecotaxe_classification_id" />
<field name="force_ecotaxe_amount" />
<field name="ecotaxe_amount" />
Copy link
Member

Choose a reason for hiding this comment

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

Same here for name field


product_id = fields.Many2one("product.product", string="Product", readonly=True)
currency_id = fields.Many2one("res.currency", string="Currency")
ecotaxe_classification_id = fields.Many2one(
Copy link
Member

Choose a reason for hiding this comment

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

Because it's an ecotaxe model, maybe classification_id name is sufficient here

"account.ecotaxe.classification",
string="Ecotaxe Classification",
)
ecotaxe_amount_unit = fields.Monetary(
Copy link
Member

@bealdav bealdav Dec 20, 2023

Choose a reason for hiding this comment

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

Because it's an ecotaxe model, maybe amount_unit name is sufficient here.

The same for ecotaxe name in fields below

"ecotaxe.line.product",
compute="_compute_all_ecotaxe_line_product_ids",
search="_search_all_ecotaxe_line_product_ids",
string="Additional ecotaxe lines",

Choose a reason for hiding this comment

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

Suggested change
string="Additional ecotaxe lines",
string="All ecotaxe lines",

To avoid Warning : Two fields (all_ecotaxe_line_product_ids, additional_ecotaxe_line_product_ids) of product.product() have the same label: Additional ecotaxe lines.

Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

@metaminux metaminux left a comment

Choose a reason for hiding this comment

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

LGTM too

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

mourad-ehm and others added 8 commits March 4, 2024 11:16
[14.0][IMP] l10n_fr_ecotaxe: manage multi ecotaxe classification by product

[14.0][FIX] l10n_fr_ecotaxe: pre-commit, access rule, view

[14.0][FIX] l10n_fr_ecotaxe: imp multi ecotaxe classification on product form & edit ecotaxe line on account move line

[14.0][FIX] l10n_fr_ecotaxe: add archived web_ribbon on ecotaxe_classification & ecotaxe_category

[14.0][FIX] l10n_fr_ecotaxe: fix tests

[14.0][FIX] l10n_fr_ecotaxe: fix _onchange_product_ecotaxe_line

[16.0][FIX] fix pre-commit & typo

[16.0][FIX] update readme files
[16.0][FIX] product variant view

[16.0][FIX] add unique sql constaint for ecotaxe classif on product
[16.0][IMP]  rename fields in l10n_fr_ecotaxe module rest of modif & imp product view
[16.0][FIX] fix psot-migration view load bug

[16.0][FIX] fix digits precision for ecotaxe amount on invoice & product & fixed ecotaxe

[16.0][FIX] tests

[16.0][FIX] replace onchange by compute

[16.0][FIX] pre-commit
@gurneyalex
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-448-by-gurneyalex-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e387934 into OCA:16.0 Jun 5, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 5712b78. Thanks a lot for contributing to OCA. ❤️

@bealdav
Copy link
Member

bealdav commented Jun 5, 2024

Oops these modules has been hardly modified here

OCA/account-fiscal-rule#428
OCA/account-fiscal-rule#429

Inside Ecotax use tax odoo mechanism

@gurneyalex
Copy link
Member

hello @bealdav sorry I had missed the discussion on #483 and the PRs you mention.

I just updated #373 and #487 with the information of the moving the modules.

I'm not sure what the best course is here.

@gurneyalex
Copy link
Member

#552 is reverting this

@bealdav
Copy link
Member

bealdav commented Jun 5, 2024

It's our own fault. We had not a really good communication on it, sorry.

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.