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][IMP] account_product_fiscal_classification: add sequence #379

Conversation

alexis-via
Copy link

No description provided.

@OCA-git-bot
Copy link
Contributor

Hi @legalsylvain,
some modules you are maintaining are being modified, check this out!

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

minor remarks. Otherwise, LGTM.
thanks !

@@ -8,6 +8,7 @@
class AccountProductFiscalClassificationTemplate(models.Model):
_name = "account.product.fiscal.classification.template"
_description = "Fiscal Classification Template"
_order = "sequence, id"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_order = "sequence, id"
_order = "sequence, name"

if two items have the same sequence. (that occures, when updating this module), better to order by name.

Copy link
Author

Choose a reason for hiding this comment

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

In the current implementation, there is no "_order", so it is order by id. So "sequence, id" keeps the current order on existing installations.

Copy link
Contributor

@legalsylvain legalsylvain Oct 23, 2023

Choose a reason for hiding this comment

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

indeed, but is it what we want ?

Copy link
Author

Choose a reason for hiding this comment

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

yes ! That way, we don't change the habits of the users until they decide the exact order they want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, order by id is wrong, and has been fixed by #275 monthes ago.
But this patch has not been back-ported.
I think it should.
the order by sequence is OK, but please take the opportunity to backport this trivial patch !

thanks !

@@ -9,6 +9,7 @@
class AccountProductFiscalClassification(models.Model):
_name = "account.product.fiscal.classification"
_description = "Fiscal Classification"
_order = "sequence, id"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_order = "sequence, id"
_order = "sequence, name"

same here.

@alexis-via alexis-via force-pushed the 16-account_product_fiscal_classification-sequence branch from c790c89 to 836e383 Compare October 23, 2023 11:50
@alexis-via
Copy link
Author

alexis-via commented Nov 2, 2023

On the form view of account.product.fiscal.classification, we have a "notebook" with a single "page"... why ? There are other modules which add tabs to this form view ?

@legalsylvain
Copy link
Contributor

we have a with a single ... why ?

?

@alexis-via
Copy link
Author

comment updated. Github dislikes when you write XML tags directly in the text of the comment!

@alexis-via
Copy link
Author

Ready to merge !

@dreispt
Copy link
Member

dreispt commented Nov 28, 2023

@legalsylvain Can you approve please, so we follow due process?

@legalsylvain
Copy link
Contributor

Hi !

On the form view of account.product.fiscal.classification, we have a "notebook" with a single "page"... why ? There are other modules which add tabs to this form view ?

Generally, I add a notebook on form view. It allows to add extra tab in custom / OCA modules.

@legalsylvain Can you approve please, so we follow due process?

No, my comment (#379 (comment)) is not answered.

But no problem : I backported #275 here (#388), so this PR has just to be rebase, and could be merged.

@alexis-via alexis-via force-pushed the 16-account_product_fiscal_classification-sequence branch from 836e383 to cf933f7 Compare January 15, 2024 10:16
@alexis-via
Copy link
Author

rebased

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 19, 2024
@github-actions github-actions bot closed this Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants