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

[ADD] cooperator_worker_force #461

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Conversation

robinkeunen
Copy link
Member

On partner form, tick "Force Worker" box to set this partner
as a worker even if they are not an effective cooperator.

Checklist before approval

  • Tests are present (or not needed).
  • Credits/copyright have been changed correctly.
  • Change log snippet is present.
  • (If a new module) Moving this to OCA has been considered.

@robinkeunen
Copy link
Member Author

@tfrancoi I moved all the worker_store relayed stuff in a separated module. This should not affect polln but it might be a good idea to have a look.

@robinkeunen
Copy link
Member Author

@polchampion This is deployed on test server (except demain). The Force Worker functionality was moved to a new module cooperator_worker_force. I suggest to test

  1. that the computation of is_worker is still correct before installing the new module
  2. that the computation of is_worker takes worker_store (Force Worker) into account after installing the module.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Merging #461 (f80c887) into 12.0 (fa7679b) will increase coverage by 21.50%.
The diff coverage is 80.00%.

@@             Coverage Diff             @@
##             12.0     #461       +/-   ##
===========================================
+ Coverage   39.26%   60.76%   +21.50%     
===========================================
  Files          44      153      +109     
  Lines        1541     5251     +3710     
  Branches      293      926      +633     
===========================================
+ Hits          605     3191     +2586     
- Misses        933     1917      +984     
- Partials        3      143      +140     
Impacted Files Coverage Δ
cooperator_worker_force/models/res_partner.py 60.00% <60.00%> (ø)
beesdoo_shift/models/res_partner.py 41.37% <100.00%> (+14.02%) ⬆️
cooperator_worker/models/res_partner.py 100.00% <100.00%> (+70.96%) ⬆️
cooperator_worker_force/models/__init__.py 100.00% <100.00%> (ø)
cooperator_worker_force/tests/__init__.py 100.00% <100.00%> (ø)
cooperator_worker_force/tests/test_worker_store.py 100.00% <100.00%> (ø)
polln_shift/models/res_partner.py 50.00% <100.00%> (+8.33%) ⬆️
beesdoo_shift_swap/models/shift_swap.py 63.41% <0.00%> (ø)
...eesdoo_shift_swap/tests/test_beesdoo_shift_swap.py 100.00% <0.00%> (ø)
beesdoo_shift_swap/models/dated_template.py 26.36% <0.00%> (ø)
... and 113 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@polchampion
Copy link
Collaborator

@robinkeunen Sucessfully tested on lepedalo-test

  • the computation of is_worker is correct before and after installing the new module
  • the computation of is_worker takes worker_store (Force Worker) into account after installing the module.
  • the module seems compatible with share operations involving change in share that allow owner to work and not.

Copy link
Member

@victor-champonnois victor-champonnois left a comment

Choose a reason for hiding this comment

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

Just one comment, I am not finished ;)

cooperator_worker_force/models/res_partner.py Show resolved Hide resolved
Copy link
Member

@victor-champonnois victor-champonnois left a comment

Choose a reason for hiding this comment

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

LGTM.

About the whole worker/allow_working logic, wouldn't it be simpler to set is_worker as a related field from the share's allow_working ?

  • The field related field could be stored, so we wouldn't have to write search methods
  • The field could be not readonly, so it could be modified on the interface.

@robinkeunen
Copy link
Member Author

I would be a simpler design indeed but a cooperator can have multiple shares so a simple related field won't do. We could have a "main share" field like "main seller" but it would be part of bigger refactoring which is already part of issue OCA/cooperative#18

@victor-champonnois
Copy link
Member

I would be a simpler design indeed but a cooperator can have multiple shares so a simple related field won't do. We could have a "main share" field like "main seller" but it would be part of bigger refactoring which is already part of issue OCA/cooperative#18

Ah yes, forgot about that 👍

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

@robinkeunen
Copy link
Member Author

/ocabot merge major

@github-grap-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 12.0-ocabot-merge-pr-461-by-robinkeunen-bump-major, awaiting test results.

github-grap-bot added a commit that referenced this pull request Nov 2, 2022
Signed-off-by robinkeunen
@github-grap-bot
Copy link
Contributor

@robinkeunen your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-461-by-robinkeunen-bump-major.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@robinkeunen
Copy link
Member Author

/ocabot merge major

@github-grap-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-461-by-robinkeunen-bump-major, awaiting test results.

github-grap-bot added a commit that referenced this pull request Nov 7, 2022
Signed-off-by robinkeunen
@github-grap-bot
Copy link
Contributor

@robinkeunen your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-461-by-robinkeunen-bump-major.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@github-grap-bot
Copy link
Contributor

@robinkeunen your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-461-by-robinkeunen-bump-major.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@robinkeunen
Copy link
Member Author

/ocabot rebase

@github-grap-bot
Copy link
Contributor

Congratulations, PR rebased to 12.0.

@robinkeunen
Copy link
Member Author

/ocabot merge major

@github-grap-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 12.0-ocabot-merge-pr-461-by-robinkeunen-bump-major, awaiting test results.

@github-grap-bot github-grap-bot merged commit 4e81438 into 12.0 Nov 14, 2022
@github-grap-bot github-grap-bot deleted the 12.0-cooperator_worker_force branch November 14, 2022 14:11
@github-grap-bot
Copy link
Contributor

Congratulations, your PR was merged at 775bb55. Thanks a lot for contributing to beescoop. ❤️

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.

6 participants