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

[14.0][FIX] project_milestone : Wrong assignment of tasks to milestones when duplicating a project #1283 #1284

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

abenzbiria
Copy link

@abenzbiria abenzbiria commented May 17, 2024

Fix for issue #1283

@OCA-git-bot
Copy link
Contributor

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

@abenzbiria
Copy link
Author

abenzbiria commented May 20, 2024

@OCA/project-maintainers : Here is a fix for issue #1283
Thank you for your review.

@abenzbiria abenzbiria changed the title [14.0][FIX] project_milestone : Wrong assignment of tasks to milestes when duplicating a project #1283 [14.0][FIX] project_milestone : Wrong assignment of tasks to milestones when duplicating a project #1283 May 20, 2024
@abenzbiria
Copy link
Author

Hi @aleuffre , @leemannd

Could you review the code of this PR, please?

Have a nice day

Copy link

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Code review, LGTM.

I wonder if there's a more robust way of finding the milestone instead of using the name. Maybe instead of copy=True the milestones could be copied "manually" inside the copy function, in order to have a mapping between old and new milestones.

milestone = project.milestone_ids.filtered(
lambda milestone: "2" not in milestone.name
)
assert tasks[0].milestone_id == milestone and tasks[1].milestone_id == milestone

Choose a reason for hiding this comment

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

chore (non-blocking): would prefer the usage of self.assertTrue or similar instead of the raw assert

@TumbaoJu
Copy link

@OCA/project-service-maintainers : Thank you for your review

Copy link
Contributor

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

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

How test fixes the assignment on project duplication?

Missed other commits, disregard

Copy link
Contributor

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

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

Code LGTM

@leemannd
Copy link

leemannd commented Jul 9, 2024

Hello @abenzbiria , the pre-commit is currently failing. Could you have a look a it?

@TumbaoJu
Copy link

TumbaoJu commented Jul 9, 2024

@majouda Can you look at the pre-commit?

@abenzbiria
Copy link
Author

The precommit is good now @leemannd @OCA/project-service-maintainers

Thanks

Copy link
Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

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

Please squash your commits

…es when duplicating a project OCA#1283

[14.0][FIX] project_milestone : Wrong assignment of tasks to milestones when duplicating a project OCA#1283

[14.0][FIX] project_milestone : Wrong assignment of tasks to milestones when duplicating a project OCA#1283

[14.0][FIX] project_milestone : Wrong assignment of tasks to milestones when duplicating a project OCA#1283

[14.0][FIX] project_milestone : Wrong assignment of tasks to milestones when duplicating a project OCA#1283

Update test_project_milestone.py

Update test_project_milestone.py
@abenzbiria
Copy link
Author

HI @OCA/project-service-maintainers , @dreispt

I squached my commits .

Thanks for your review

@TumbaoJu
Copy link

TumbaoJu commented Sep 9, 2024

@OCA/project-service-maintainers : Hello! We think that everything is ok in the PR. Can one of you revised it and merge it?
Thank you in advance.

@dreispt
Copy link
Member

dreispt commented Sep 12, 2024

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-1284-by-dreispt-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 999c349 into OCA:14.0 Sep 12, 2024
7 of 9 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

7 participants