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.

Sign up for GitHub

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

Jump to bottom

[14.0][ADD] project_sequence #1101

Merged
merged 5 commits into from
May 11, 2023
Merged

[14.0][ADD] project_sequence #1101

merged 5 commits into from
May 11, 2023

Conversation

anddago78
Copy link

Add a sequence field to projects, filled automatically and add a code sequence filter in tree view project.

MT-1506 @moduon @rafaelbn @Shide @yajo please review :)

@anddago78 anddago78 force-pushed the 14.0-project_sequence branch 3 times, most recently from 497794b to b5d611a Compare April 10, 2023 08:20
yajo
yajo requested changes Apr 10, 2023
View reviewed changes
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

It's OK in general. It just has a lot of little details to improve. See suggestions below. Thanks!

README.md Outdated Show resolved Hide resolved
project_sequence/data/ir_sequence.xml Outdated Show resolved Hide resolved
project_sequence/data/ir_sequence.xml Outdated Show resolved Hide resolved
project_sequence/data/ir_sequence.xml Outdated Show resolved Hide resolved
project_sequence/i18n/.empty Outdated Show resolved Hide resolved
project_sequence/views/project_project.xml Outdated Show resolved Hide resolved
project_sequence/views/project_project.xml Outdated Show resolved Hide resolved
project_sequence/views/project_project.xml Outdated Show resolved Hide resolved
project_sequence/models/project_project.py Outdated Show resolved Hide resolved
project_sequence/models/project_project.py Outdated Show resolved Hide resolved
@anddago78 anddago78 force-pushed the 14.0-project_sequence branch 2 times, most recently from 4072367 to e2e4c2a Compare April 10, 2023 09:34
fcvalgar
fcvalgar reviewed Apr 10, 2023
View reviewed changes
Copy link

@fcvalgar fcvalgar left a comment

Choose a reason for hiding this comment

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

After my functional review I detect that each project creation generates a new sequence.

I think what you wanted to do @anddago78 is to set a single sequence to project creation.

Can you check if this point is correct. Thank you.

Captura desde 2023-04-10 12-41-03

@anddago78 anddago78 marked this pull request as draft April 11, 2023 06:34
@anddago78 anddago78 force-pushed the 14.0-project_sequence branch 8 times, most recently from 073c06a to fd2f76a Compare April 12, 2023 06:25
@anddago78 anddago78 force-pushed the 14.0-project_sequence branch 5 times, most recently from 1496b6f to 743fa96 Compare April 18, 2023 12:32
@anddago78 anddago78 requested a review from yajo April 18, 2023 12:37
@yajo yajo marked this pull request as ready for review April 19, 2023 08:10
yajo
yajo requested changes Apr 19, 2023
View reviewed changes
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

This looks quite good. I found some improvements though, could you take a look please? Thanks! 😊

project_sequence/data/ir_sequence.xml Outdated Show resolved Hide resolved
project_sequence/i18n/es.po Outdated Show resolved Hide resolved
project_sequence/i18n/es.po Outdated Show resolved Hide resolved
project_sequence/i18n/es.po Outdated Show resolved Hide resolved
project_sequence/models/project_project.py Outdated Show resolved Hide resolved
project_sequence/models/project_project.py Outdated Show resolved Hide resolved
project_sequence/models/project_project.py Outdated Show resolved Hide resolved
@anddago78 anddago78 force-pushed the 14.0-project_sequence branch 5 times, most recently from 0cb5506 to 48824ec Compare April 19, 2023 13:32
@yajo yajo requested a review from rafaelbn May 9, 2023 10:06
@yajo
Copy link
Member

yajo commented May 10, 2023

If runboat fails, you have to uninstall project_wbs. It introduces some incompatibilities. I have recorded that incompatibility in the manifest, so you can't install both and the problem is avoided.

@yajo yajo mentioned this pull request May 10, 2023
Open
yajo added 2 commits May 10, 2023 11:11
@yajo
[IMP] project_sequence: move sequence below name
8c9e29c 
When a project has a name, still the sequence is a very important field to be displayed. I move it below the project name.

@moduon MT-1506
@yajo
[FIX] project_wbs: make compatible with project_sequence
113593b 
Support the use case of a `False` name coming in, which can happen when installing `project_sequence`.

@moduon MT-1506
@yajo
[FIX] project_sequence: make compatible with hr_timesheet
6835c70 
`hr_timesheet` creates an analytic account by default. The method it uses to create it expects a preexisting name. But since we're making name not required, we're breaking other module's expectations.

To fix this problem, now the name sync is done before writing or creating records, and not after.

To make sure the problem doesn't happen anymore, we keep the `NOT NULL` requirement on project names. We just do it with a manual SQL constraint. This extra protection ensures compatibility with any other modules that expect always a value on the name.

Any possibly misconfigured sequence could produce sequence duplicates. I also add protection against that.

In tests, the project sequence was wrongly reset to 11 only once. Turns out that it survives the test savepoint, so I now reset it before each test instead. Tests are more reliable now. Besides, I added some more.

@moduon MT-1506

wip

wip
@yajo
Copy link
Member

yajo commented May 10, 2023

Due to sbidoul/runboat#36, introducing the incompatibility with project_wbs made runboat die.

Now, I've fixed project_wbs so that it's resilient to the installation of project_sequence. Runboat should give no more problems and CI is ✅ now.

@yajo yajo dismissed rafaelbn’s stale review May 10, 2023 11:40

Your review is outdated, please review again

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

fcvalgar
fcvalgar approved these changes May 10, 2023
View reviewed changes
Copy link

@fcvalgar fcvalgar left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @yajo

rafaelbn
rafaelbn reviewed May 10, 2023
View reviewed changes
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.

Commented with @fcvalgar , display name standard without sequence

@yajo
[FIX] project_sequence: support preexisting sequenceless projects
6ac7e10 
These projects shouldn't display "False - Project name" in their display names.

@moduon MT-1506
rafaelbn
rafaelbn approved these changes May 11, 2023
View reviewed changes
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.

👍🏼 Thanks

An improvement por generalización will come before migration to 15

@rafaelbn
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-1101-by-rafaelbn-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4b02952 into OCA:14.0 May 11, 2023
@OCA-git-bot
Copy link
Contributor

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

@yajo yajo deleted the 14.0-project_sequence branch May 12, 2023 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yajo yajo yajo approved these changes

@Shide Shide Shide approved these changes

@rafaelbn rafaelbn rafaelbn approved these changes

@fcvalgar fcvalgar fcvalgar approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 needs review ready to merge
Projects
None yet
Milestone
14.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@anddago78 @yajo @OCA-git-bot @rafaelbn @Shide @fcvalgar