[16.0][MIG] project_task_code #1084

Abranes · 2023-03-09T09:41:59Z

Supersedes #1013

pedrobaeza · 2023-03-09T10:46:16Z

/ocabot migration project_task_code

OCA-git-bot · 2023-03-09T10:46:26Z

The migration issue (#988) has been updated to reference the current pull request.
however, a previous pull request was referenced : #1013.
Perhaps you should check that there is no duplicate work.
CC : @uncannycs

rafaelbn · 2023-03-27T19:48:41Z

Hello @Abranes

Please this module in 15 (at least) is creating constraint that breaks runboat with other modules. could you FIX it here in migration?

@Shide

Yes, project_task_code module is creating a constraint on project.task and other modules who creates tasks (by strange ways or on a different order) breaks the init of the ruboat with demo data.

@sbidoul

Ah, yes. IMO modules that add constraints on standard models should enable them by configuration, otherwise we reach dead ends like this one.

#1096 (comment)

Thank you!

Abranes · 2023-04-21T16:04:22Z

Hello @Abranes

Please this module in 15 (at least) is creating constraint that breaks runboat with other modules. could you FIX it here in migration?

@Shide

Yes, project_task_code module is creating a constraint on project.task and other modules who creates tasks (by strange ways or on a different order) breaks the init of the ruboat with demo data.

@sbidoul

Ah, yes. IMO modules that add constraints on standard models should enable them by configuration, otherwise we reach dead ends like this one.

#1096 (comment)

Thank you!

Done! You can review it.

Shide

Code is being set even if in the settings, the Task Code Unique is not checked.
Also check the pre_init_hook and post_init_hook

project_task_code/hooks.py

Shide · 2023-04-25T06:06:50Z

project_task_code/models/project_task.py

+
+    @api.model_create_multi
+    def create(self, vals_list):
+        for vals in vals_list:


Should be done if user has activated the corresponding parameter in res.config.settings and sequence exists in self.env.company.

project_task_code/models/res_config_settings.py

project_task_code/models/project_task.py

rousseldenis · 2023-06-06T13:51:24Z

@Abranes

project_task_code/views/project_view.xml

yaazkal · 2023-10-12T15:51:46Z

Is there something pending here still?

Shide · 2023-10-16T07:16:00Z

Is there something pending here still?

Check @jort-aardug and @Shide suggestions

yaazkal · 2023-10-21T15:27:14Z

@Abranes can you check the suggestions for this to be merged? Thanks

robinkeunen · 2023-10-26T15:30:39Z

I made a suggestion for the name tag comment. If I understand correctly, the project_task_unique_code parameter adds the check for unicity, it does not activate the feature. If so, @Shide 's comment do not apply.

I can do a superseeding PR if @Abranes or maintainers give me the go.

Abranes · 2023-10-27T20:52:15Z

Sorry guys, I forgot this PR.

I'll try to address all points next week.

Abranes · 2023-11-02T11:56:09Z

@jort-aardug @Shide @robinkeunen @yaazkal could you check it one more time? :)

Shide

praise: Good job with this man :)

Code LGTM

OCA-git-bot · 2023-11-02T13:00:02Z

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

robinkeunen

I think there is a confusion here, the module should

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

The last fixes does the opposite:

force the unicity constraint upon installation
create the task code on configuration

Abranes · 2023-11-02T13:57:52Z

I think there is a confusion here, the module should

add the task code upon installation

force the unicity constraint on configuration

The last fixes does the opposite:

force the unicity constraint upon installation

create the task code on configuration

It's what @Shide requests and I think it's not a bad approach.

robinkeunen · 2023-11-02T14:05:36Z

But that changes the purpose of the module, migration is not the good time to do that. Plus a better module name would then me project_task_code_unique.

Plus it does not follow @sbidoul and @rafaelbn 's recommendation. Let's see what others think.

robinkeunen · 2023-12-15T13:01:59Z

Hi @rousseldenis @dreispt , we're disagreeing on the way to go with this PR. Can you help us move forward ?

Shide · 2023-12-15T16:35:04Z

But that changes the purpose of the module, migration is not the good time to do that. Plus a better module name would then me project_task_code_unique.

Plus it does not follow @sbidoul and @rafaelbn 's recommendation. Let's see what others think.

Maybe we should add a migration script?

The module without this fix/feature will break tests for sure at any point in the future... If someone has the bad luck of having this module installed in another way.

rafaelbn · 2023-12-15T19:00:32Z

Hello,

I don't know in this point which is the disagree. I pinged @Abranes because this module is breaking other modules test, etc... and we said to FIX it here.

Hello @Abranes

Please this module in 15 (at least) is creating constraint that breaks runboat with other modules. could you FIX it here in migration?

@Shide
Yes, project_task_code module is creating a constraint on project.task and other modules who creates tasks (by strange ways or on a different order) breaks the init of the ruboat with demo data.
@sbidoul
Ah, yes. IMO modules that add constraints on standard models should enable them by configuration, otherwise we reach dead ends like this one.
#1096 (comment)

Thank you!

The request was to address a constraint issue in the project_task_code module for version 15. The module's constraint on project.task is causing conflicts with other modules in runboat environments, especially those creating tasks in unconventional ways. @sbidoul suggests that modules adding constraints to standard models should have the option to enable these constraints through configuration to avoid such conflicts. This suggestion aims to prevent dead ends and improve module compatibility.

We are not using this module at all, just we are affected in other modules un runboat and test because this one.

@robinkeunen > But that changes the purpose of the module
@Shide > The module without this fix/feature will break tests for sure at any point in the future... If someone has the bad luck of having this module installed in another way.

The purpose of the module shouln't be change but FIXED

Maybe @oihane or @pedrobaeza as original contributors what to put an opinion. I don't know exacly what's the problem now. @dreispt Do you have a solution here o some past use case?

Thank you all! 😄 ❤️

pedrobaeza · 2023-12-15T19:09:03Z

Can you summarize instead of copying a lot of texts? If I understand correctly, there was a constraint and it has been removed in the migration? Or the contrary? Which is the problem in runboat?

robinkeunen · 2023-12-16T11:07:54Z

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 from @Abranes, based on @Shide's request, does the opposite, it

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

pedrobaeza · 2023-12-16T17:11:30Z

Thanks for the summary, Robin.

It's true that enforcing a uniqueness can be conflicting for prior version databases not containing this constraint migrating to this version. If implemented via SQL constraint, Postgres will try to put the constraint and fail, not having the constraint at all, but only a warning each time you update. Doing that by Python constraint allows to be enabled by configuration, and this would be a saner feature.

Another conflicting change is to fill existing taxes with a code, as you may not want to have one at all. I would go for having a button on configuration to fill the code on "empty code" taxes on demand.

Do you see it very complicated to be done?

huguesdk · 2023-12-19T15:55:05Z

i don’t understand why so many changes are introduced in a simple version migration. this sql constraint is present since version 8. the hooks that run upon module initialization ensure that this constraint is satisfied, and that tasks created subsequently (even by other modules) will have a unique code (coming from a sequence). it shouldn’t cause an issue.

i don’t think it’s a good idea to introduce such significant changes as part of a version migration, unless something that changed in that version prevents the module from working as it did in the previous version.

i think that the changes in commit 90103ce are enough.

robinkeunen · 2023-12-20T14:36:59Z

Rather than arguing, I propose this PR #1209. I leave it to the PSC to choose what is more appropriate :-)

huguesdk · 2023-12-20T16:39:10Z

could someone give a more detailed explanation about what the problem that we’re trying to solve exactly is (apart from porting this module to 16)? i’ve read that it caused errors during the runboat build. could someone copy the exact error here to see if it can be solved in a simpler way?

note that the original sql constraint is not multi-company compatible (it requires uniqueness of the code while it should probably be unique per company) while sequences are per company by default. maybe this could be the issue?

legalsylvain · 2023-12-20T20:09:36Z

Hi @rafaelbn

Please this module in 15 (at least) is creating constraint that breaks runboat with other modules. could you FIX it here in migration?

could you mention the error generated ? I don't understand how it can break things !

Thanks !

Abranes · 2023-12-21T11:35:45Z

Hi all! I can make the changes you want but first let's all be clear, because in the first instance I perform a normal migration, then modify the module so that the constraint was not by SQL and was by python and thus be able to activate or deactivate it in the configuration to avoid problems with other modules, and finally carry out the current implementation, all at the request of different people.

How do you prefer it to proceed?

huguesdk · 2023-12-22T09:09:13Z

i personally think that the sql constraint should stay. it is the only way to ensure uniqueness in a reliable way. checking it in python won’t prevent the same code to be used if records are created at the same time. we need to check what problem the sql constraint actually caused, and fix it without removing the constraint.

rafaelbn · 2023-12-26T17:03:49Z

Thinking in an OCA Repo with more modules involved:

IMO modules that add constraints on standard models should enable them by configuration, otherwise we reach dead ends. So not by SQL but by python and thus be able to activate or deactivate it in the configuration to avoid problems with other modules

Not blocking anything, just my opinion.

Thanks all by efforts in the best way! 😄 ❤️

legalsylvain · 2023-12-26T17:57:31Z

IMO modules that add constraints on standard models should enable them by configuration, otherwise we reach dead ends. So not by SQL but by python and thus be able to activate or deactivate it in the configuration to avoid problems with other modules

Hi. my opinion is "Keep It simple". An SQL constraint is way more simple as constraint enabled by configuration. So, if there is no existing problem, I propose to keep the V15 code.

https://github.com/OCA/project/blob/15.0/project_task_code/models/project_task.py#L18-L20 is simpler (2 lines) than :

@rafaelbn : you was saying that V15 runboat was broken, but I see it green. Is it fixed ? https://runboat.odoo-community.org/webui/builds.html?repo=OCA/project&target_branch=15.0

rafaelbn · 2023-12-29T13:42:38Z

Hello @legalsylvain! Happy new year 2024! 😄

@Shide will explain after holidays the problem that this modules causes when living in the same repo with other ones.

We are not using this module at all, we just found problems when working in others in the same repo.

Thank you!

Shide · 2024-01-10T10:35:23Z

@legalsylvain the case which i'm saying this needs a change is because this happens to me in this PR: #1096 (comment)

Check out the next 4 comments where i'd invoked you because the runboat breaks

legalsylvain · 2024-01-10T10:53:21Z

Hi @Shide. Thanks a lot for pointing the issue. @robinkeunen, could you take a look at this issue ?
thanks !
and happy new years !

rafaelbn · 2024-01-13T20:44:31Z

This PR is continued in #1209

Abranes force-pushed the 16.0-mig-project_task_code branch from baa5a78 to 1329761 Compare March 9, 2023 09:46

Abranes mentioned this pull request Mar 9, 2023

16.0 mig project task code #1013

Closed

Abranes force-pushed the 16.0-mig-project_task_code branch from 1329761 to 01f2b27 Compare March 9, 2023 09:52

OCA-git-bot added this to the 16.0 milestone Mar 9, 2023

OCA-git-bot mentioned this pull request Mar 9, 2023

Migration to version 16.0 #988

Open

38 tasks

Abranes force-pushed the 16.0-mig-project_task_code branch from 01f2b27 to 90103ce Compare March 16, 2023 10:49

Abranes force-pushed the 16.0-mig-project_task_code branch from 90103ce to f1a8517 Compare April 21, 2023 16:03

Shide suggested changes Apr 25, 2023

View reviewed changes

rousseldenis added needs review migration labels Jun 6, 2023

yostashiro mentioned this pull request Jul 27, 2023

[16.0][ADD] project_task_name_with_id #1158

Merged

jort-aardug reviewed Aug 1, 2023

View reviewed changes

project_task_code/views/project_view.xml Outdated Show resolved Hide resolved

robinkeunen approved these changes Oct 20, 2023

View reviewed changes

oihane and others added 7 commits November 2, 2023 10:55

[MOD] changing from odoomrp to avanzosc

f67052b

[FIX] Code error when copying a task

34b36a3

[IMP] <project_task_code> adapted to OCA

d9869de

[IMP] <project_task_code> Added docstring to pre and post init-hooks

18bb887

[UPD] prefix versions with 8.0

ea1c0bd

[MIG] Make modules uninstallable

2bcc7f0

[MIG] Rename manifest files

d8a00ff

Shide approved these changes Nov 2, 2023

View reviewed changes

OCA-git-bot added approved ready to merge labels Nov 2, 2023

robinkeunen suggested changes Nov 2, 2023

View reviewed changes

OCA-git-bot removed approved ready to merge labels Nov 2, 2023

robinkeunen mentioned this pull request Dec 20, 2023

[16.0][MIG] project_task_code : alternative without functionality change #1209

Merged

Abranes closed this Jan 10, 2024

[16.0][MIG] project_task_code #1084

[16.0][MIG] project_task_code #1084

Conversation

Abranes commented Mar 9, 2023

pedrobaeza commented Mar 9, 2023

OCA-git-bot commented Mar 9, 2023

rafaelbn commented Mar 27, 2023 • edited Loading

Abranes commented Apr 21, 2023

Shide left a comment

Choose a reason for hiding this comment

Shide Apr 25, 2023

Choose a reason for hiding this comment

rousseldenis commented Jun 6, 2023

yaazkal commented Oct 12, 2023

Shide commented Oct 16, 2023

yaazkal commented Oct 21, 2023

robinkeunen commented Oct 26, 2023

Abranes commented Oct 27, 2023

Abranes commented Nov 2, 2023

Shide left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Nov 2, 2023

robinkeunen left a comment • edited Loading

Choose a reason for hiding this comment

Abranes commented Nov 2, 2023

robinkeunen commented Nov 2, 2023 • edited Loading

robinkeunen commented Dec 15, 2023

Shide commented Dec 15, 2023 • edited Loading

rafaelbn commented Dec 15, 2023

pedrobaeza commented Dec 15, 2023

robinkeunen commented Dec 16, 2023

pedrobaeza commented Dec 16, 2023

huguesdk commented Dec 19, 2023

robinkeunen commented Dec 20, 2023

huguesdk commented Dec 20, 2023

legalsylvain commented Dec 20, 2023

Abranes commented Dec 21, 2023

huguesdk commented Dec 22, 2023

rafaelbn commented Dec 26, 2023

legalsylvain commented Dec 26, 2023 • edited Loading

rafaelbn commented Dec 29, 2023

Shide commented Jan 10, 2024 • edited Loading

legalsylvain commented Jan 10, 2024

rafaelbn commented Jan 13, 2024

rafaelbn commented Mar 27, 2023 •

edited

Loading

robinkeunen left a comment •

edited

Loading

robinkeunen commented Nov 2, 2023 •

edited

Loading

Shide commented Dec 15, 2023 •

edited

Loading

legalsylvain commented Dec 26, 2023 •

edited

Loading

Shide commented Jan 10, 2024 •

edited

Loading