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

[FIX] bad states in decoration-info of purchase.order tree view #70158

Closed
wants to merge 1 commit into from

Conversation

legalsylvain
Copy link
Contributor

@legalsylvain legalsylvain commented Apr 30, 2021

Trivial patch.

Set correct decoration-info attribute on purchase.order tree view, replacing unexisting states ('wait','confirmed') by existing ones ('draft','sent')

See list of valid states : https://github.com/odoo/odoo/blob/master/addons/purchase/models/purchase.py#L97

Note : this bug is present since at least V12.0

CC : @Yenthe666, @quentinDupont

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@legalsylvain legalsylvain requested a review from a team as a code owner April 30, 2021 09:59
@robodoo
Copy link
Contributor

robodoo commented Apr 30, 2021

@amoyaux
Copy link
Contributor

amoyaux commented May 3, 2021

Okay, can you please rewrite the commut message following the guidelines https://www.odoo.com/documentation/14.0/reference/guidelines.html#git

@legalsylvain
Copy link
Contributor Author

Hi @amoyaux. Thanks for your review and your answer.

  • I rewrote the short sentence, adding the module name.
  • I didn't added references at the end of the message, because there are no related bug / task / opw related. (AFAIK)

Let me know if it's OK.

One question :

The bug is present since many versions. (present at least in V12). What is the best for odoo team ? do a PR against 12.0 branch ? master branch ? all the branches ? I don't understand how robodoo works, to find the branches into merge the PR.

kind regards.

@amoyaux
Copy link
Contributor

amoyaux commented May 3, 2021

@legalsylvain Don't worry for the references, not needed in your case.

For your question, it depends, when you create a PR for a specific version, we have an automated tools call forward-port bot that will try apply the fix for all the version between the PR branch up to master. https://github.com/odoo/odoo/wiki/Mergebot#forward-port

For example if you do a PR in 12.0, the bot will create a PR for branch:
saas-12.3 -> 13.0 -> saas-13.3 -> 14.0 -> saas-14.3 -> master
If you do the PR in 14.0:
saas-14.3 -> master
For master there will be no forward port since you are already on the latest version

Usually, we prefer that people propose the fix on the version they need it rather than bellow because we are never safe from a bug and if other people need it, they can always suggest a back port.

@amoyaux
Copy link
Contributor

amoyaux commented May 3, 2021

@legalsylvain What do you think of moving it on the name instead of the complete line. It's a big less aggressive.

image
image

@legalsylvain
Copy link
Contributor Author

@legalsylvain What do you think of moving it on the name instead of the complete line. It's a big less aggressive.

Hi @amoyaux.

I'm currently working on 12.0 and proposed that PR only to avoid to have a new patch, after the next migration ;-)

  • In V12, it seems that the "standard" is to to have the full line in a given color
  • In more recent version (I don't use for the time being), indeed, odoo introduced color per column (for exemple, on sale module)

At the moment, I don't see your proposal consistent with the current decoration-muted="state=='cancel'" present in the line level.
The scope of this PR is just a bug fix, because the current decoration-info is related to unexisting state.
Maybe we could merge this bug fix, and then think about deeper changes on purchase tree view in another task.

Don't you think ?

@amoyaux
Copy link
Contributor

amoyaux commented May 11, 2021

Hello,

Since the PR is for master and the decoration exists for one field instead of the line, I would use it. Maybe it's not consistent with cancel but decoration-muted is less visible and often filter out of views by default.

@legalsylvain legalsylvain force-pushed the patch-6 branch 2 times, most recently from 584344a to d1a837f Compare May 11, 2021 09:20
- set correct decoration-info attribute on purchase.order tree view, replacing unexisting states ('wait','confirmed') by existing ones ('draft','sent')
- Move the decoration-info from tree line to name field to comply with the new display standards
@legalsylvain
Copy link
Contributor Author

Hello @amoyaux.

Since the PR is for master and the decoration exists for one field instead of the line, I would use it. Maybe it's not consistent with cancel but decoration-muted is less visible and often filter out of views by default.

Thanks for your answer. I so moved the decoration-info according to new Odoo convention, on name field. Is this PR ready to merge for you ?

Let me know. thanks.

@amoyaux
Copy link
Contributor

amoyaux commented May 11, 2021

@legalsylvain Yep, good that way. Thanks for the contribution.

robodoo r+

robodoo pushed a commit that referenced this pull request May 11, 2021
- set correct decoration-info attribute on purchase.order tree view, replacing unexisting states ('wait','confirmed') by existing ones ('draft','sent')
- Move the decoration-info from tree line to name field to comply with the new display standards

closes #70158

Signed-off-by: Arnold Moyaux <amoyaux@users.noreply.github.com>
@robodoo robodoo added the 14.4 label May 11, 2021
@robodoo robodoo closed this May 11, 2021
@robodoo robodoo temporarily deployed to merge May 11, 2021 12:58 Inactive
@legalsylvain
Copy link
Contributor Author

Hi @amoyaux. Thanks for your reactivity, and for merging this PR.
If purchase module is in your scope, could you take a look on this other PR ?
#70462

I am available to discuss if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants