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] project_task_code : alternative without functionality change #1209

Merged
merged 53 commits into from
Jan 13, 2024

Conversation

robinkeunen
Copy link

@robinkeunen robinkeunen commented Dec 20, 2023

This PR is an alternative to #1084.

In 15.0 : project_task_code

  • adds code field on tasks
  • adds sql contraint project_task_unique_code
  • a hook initializes code from project.task sequence
  • in create, code is assigned from project.task sequence

In his migration review (2023-03-27), @rafaelbn pointed out that the a configuration should be added for the constraint. Otherwise, it breaks the tests from other modules.

I understand from the suggestion that the module should

  • add the task code upon installation
  • force the unicity constraint on configuration

The fix in #1084 does the opposite, it

  • forces the unicity constraint upon installation
  • creates the task code on configuration

It has the advantage of making the module multi-company compliant but at the coast of - according to me - changing the functionality of the module.

I leave it to the PSC to choose the most appropriate PR :-)

@robinkeunen robinkeunen force-pushed the 16.0-mig-project_task_code-roke branch from 87b6fbc to c04a2de Compare December 20, 2023 14:38
@rafaelbn rafaelbn requested a review from Shide December 26, 2023 17:04
@rafaelbn
Copy link
Member

/ocabot migration project_task_code

@OCA-git-bot
Copy link
Contributor

The migration issue (#988) has not been updated to reference the current pull request because a previous pull request (#1084) is not closed.
Perhaps you should check that there is no duplicate work.
CC @Abranes

string="Task Number",
required=True,
default="/",
readonly=True,
Copy link
Member

Choose a reason for hiding this comment

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

Not for migration: What is the reason behind keeping this field readonly?

Copy link
Author

Choose a reason for hiding this comment

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

Because it is generated through a sequence :-)

Copy link
Member

@ivs-cetmix ivs-cetmix left a comment

Choose a reason for hiding this comment

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

Functional review LGTM. Using this module in prod already)

Copy link

@Shide Shide left a comment

Choose a reason for hiding this comment

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

I'm ok with this, but If in the future happens something that breaks runboat, will be good to modify the module.

The runboat break happens to me in this PR: #1096 (comment)

@robinkeunen
Copy link
Author

robinkeunen commented Jan 11, 2024

I'm sorry this is going back and forth that way, not trying to be a troll ... :/

I actually was convinced by @huguesdk and @legalsylvain 's comment on #1084 . I see no reason why the sql constraint would conflict w/ other module since the field code is added by this module.

I wonder if the error you had in v15 was due to the constraint checking unicity on the code but not on the company ? We'll see and fix if it rises again.

@robinkeunen
Copy link
Author

Last 3 commits should be smashed.

@robinkeunen
Copy link
Author

Hi @dreispt , can you take a look at this PR ? It raised many concerns in #1084 and #1013 . It would be great to finally settle it :-)

@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). 🤖

Copy link
Member

@huguesdk huguesdk left a comment

Choose a reason for hiding this comment

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

@robinkeunen thanks for this. would you please squash the last 3 commits?

Yvesldff and others added 16 commits January 12, 2024 17:44
Currently translated at 50.0% (3 of 6 strings)

Translation: project-14.0/project-14.0-project_task_code
Translate-URL: https://translation.odoo-community.org/projects/project-14-0/project-14-0-project_task_code/fr_FR/
Currently translated at 100.0% (3 of 3 strings)

Translation: project-14.0/project-14.0-project_task_code
Translate-URL: https://translation.odoo-community.org/projects/project-14-0/project-14-0-project_task_code/es_MX/
Currently translated at 100.0% (3 of 3 strings)

Translation: project-14.0/project-14.0-project_task_code
Translate-URL: https://translation.odoo-community.org/projects/project-14-0/project-14-0-project_task_code/sv/
Currently translated at 100.0% (3 of 3 strings)

Translation: project-15.0/project-15.0-project_task_code
Translate-URL: https://translation.odoo-community.org/projects/project-15-0/project-15-0-project_task_code/ca/
The overload of project.task::create  in the project_task_code would
mutate the dictionaries passed in the vals_list argument. This is a bad
practice which can have unintended side effects in caller code.

We fix this by creating a new dictionary containing the additional field
value and passing this dictionary in the call to super().create()
Currently, the sequence that generates task codes is only available for the
main company [1], which could cause issues at the moment of the installation
process with another modules in multi-companies cases, for example
project_timesheet_holiday [2] because in the other companies, this line [3]
will return False or "/" and will trigger the UNIQUE constraint. With
this change, the sequence can be used by any company [4] using the False
value in the field company_id.

References:
[1] https://github.com/odoo/odoo/blob/7b6c25e/odoo/addons/base/models/ir_sequence.py#L148
[2] https://github.com/odoo/odoo/blob/7b6c25e/addons/project_timesheet_holidays/models/res_company.py#L14
[3] https://github.com/OCA/project/blob/9d08f2d/project_task_code/models/project_task.py#L29
[4] https://github.com/odoo/odoo/blob/7b6c25e/odoo/addons/base/models/ir_sequence.py#L282
@robinkeunen robinkeunen force-pushed the 16.0-mig-project_task_code-roke branch from 30e58be to 2ca302e Compare January 12, 2024 16:47
@rafaelbn
Copy link
Member

/ocabot migration project_task_code

@OCA-git-bot OCA-git-bot mentioned this pull request Jan 13, 2024
38 tasks
@rafaelbn
Copy link
Member

@legalsylvain , are you PSC here to merge?

@legalsylvain
Copy link
Contributor

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

/ocabot merge patch

Merging as long discussed in other PR, but if any issue I will ping you 😄

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1209-by-rafaelbn-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit dc799f5 into OCA:16.0 Jan 13, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@robinkeunen robinkeunen deleted the 16.0-mig-project_task_code-roke branch January 15, 2024 14:07
@robinkeunen
Copy link
Author

🙏 that was a long one, thanks @Shide and @Abranes

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.