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

[13.0] [MIG] fieldservice_skill #602

Merged
merged 32 commits into from
Oct 31, 2020
Merged

[13.0] [MIG] fieldservice_skill #602

merged 32 commits into from
Oct 31, 2020

Conversation

brian10048
Copy link
Contributor

Migrate fieldservice_skill to 13.0

For #354

@brian10048 brian10048 added this to the 13.0 milestone Jul 21, 2020
@brian10048 brian10048 self-assigned this Jul 21, 2020
@OCA-git-bot OCA-git-bot mentioned this pull request Jul 21, 2020
Open
43 tasks
@brian10048 brian10048 linked an issue Jul 21, 2020 that may be closed by this pull request
Migration to version 13.0 #354
Open
43 tasks
@OCA-git-bot
Copy link
Contributor

Hi @max3903, @osi-scampbell,
some modules you are maintaining are being modified, check this out!

@bodedra
Copy link
Member

bodedra commented Jul 21, 2020

This PR depends on "hr_skill" module and its functionality come in Odoo13 hr_skills module.

Firefox_Screenshot_2020-07-21T19-02-24 423Z

So change module dependency from "hr_skill" to "hr_skills"

bodedra
bodedra suggested changes Jul 21, 2020
View reviewed changes
fieldservice_skill/__manifest__.py Outdated Show resolved Hide resolved
@brian10048
Copy link
Contributor Author

This PR depends on "hr_skill" module and its functionality come in Odoo13 hr_skills module.

Firefox_Screenshot_2020-07-21T19-02-24 423Z

So change module dependency from "hr_skill" to "hr_skills"

I updated the dependency and updated an inherited view name.

Will a migration script be needed because of this module change?

@bodedra
Copy link
Member

bodedra commented Jul 21, 2020

@brian10048 I don't think we need a migration script because it has same model name "hr.skill" So we are safe.

cc @max3903 @dreispt

@RLeeOSI RLeeOSI mentioned this pull request Oct 9, 2020
Closed
@RLeeOSI
Copy link
Contributor

RLeeOSI commented Oct 9, 2020

@max3903 @osi-scampbell

Closed my PR, I missed that this was already open. Same questions as before.

The dependency hr_skill from OCA/hr has been changed to hr_skills from odoo/odoo. The old module had a color field on hr.skill model, while the new one does not. Does this need to be added back in?

The Usage file indicates that the worker list on fsm.order will be filtered by worker skill (as well as location) but there's no functionality to filter by worker skill in the v12 module. Should this be added in?

@dreispt
Copy link
Member

dreispt commented Oct 12, 2020

🚨 Red build!

@brian10048
Copy link
Contributor Author

@RLeeOSI I saw your PR at brian10048#16, but I wonder if we should modify the v12 behavior to instead open the Skill Type view as it provides a more complete overview of default skills configuration like I recently updated here.

After looking at this more, I wonder if we should update the fsm.person.skill to more or less mimic hr.employee.skill
The Odoo skill module has a level progress feature that can replace the level priority style field on fsm.person.skill

I don't mind making the changes, just want to make sure we are all on the same page first. @dreispt @murtuzasaleh Let me know what you guys think as well

@RLeeOSI
Copy link
Contributor

RLeeOSI commented Oct 12, 2020 •
edited
Loading

@brian10048 I agree, it looks like the Odoo module intends to use hr.skill.type as the main model for skills (hence why there's a menuitem for it and not hr.skill). @max3903 and @osi-scampbell your input here would be greatly appreciated, and on the earlier comment here

@max3903
Copy link
Member

max3903 commented Oct 15, 2020

The dependency hr_skill from OCA/hr has been changed to hr_skills from odoo/odoo. The old module had a color field on hr.skill model, while the new one does not. Does this need to be added back in?

Yes

The Usage file indicates that the worker list on fsm.order will be filtered by worker skill (as well as location) but there's no functionality to filter by worker skill in the v12 module. Should this be added in?

Yes

@dreispt
Copy link
Member

dreispt commented Oct 16, 2020

@brian10048 Lint checks are failing.
Can you check if you have pre-commit configured on your local v13 repository?

@RLeeOSI RLeeOSI mentioned this pull request Oct 16, 2020
Closed
@brian10048 brian10048 force-pushed the 13.0-mig-fsm-skill branch 2 times, most recently from 4ae4a1b to a885850 Compare October 28, 2020 19:52
bodedra
bodedra approved these changes Oct 28, 2020
View reviewed changes
Copy link
Member

@bodedra bodedra left a comment

Choose a reason for hiding this comment

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

Awesome. LG 👍

@brian10048
Copy link
Contributor Author

Screenshot from 2020-10-28 16-37-08

I am implementing a cleaner look for the worker skills to match the Odoo v13 skills look for employees.

I will have that ready for a final review soon

@bodedra
Copy link
Member

bodedra commented Oct 29, 2020

@brian10048 Would you please fix this?

Screenshot from 2020-10-29 16-17-56

@brian10048
Copy link
Contributor Author

@brian10048 Would you please fix this?

Ready for final review @bodedra

@brian10048
[IMP] fieldservice_skill: Skill views
cef004c 
Adapted FSM Person Skills to mimic Odoo v13 behavior of Employee Skills.
- Skill progress level instead of priority
- Updated views on FSM Person form
@brian10048
[IMP] fieldservice_skill: Add Test
fd662dd
bodedra
bodedra approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@bodedra bodedra left a comment

Choose a reason for hiding this comment

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

@brian10048 Functional reviewed. works great! 👍

+1 for beautiful view.

Screenshot from 2020-10-30 11-06-44

@bodedra
Copy link
Member

bodedra commented Oct 30, 2020

@max3903 @dreispt would you please give your blessing to this PR!

@brian10048
Copy link
Contributor Author

Thanks @bodedra, we can thank Odoo for the design of the view. I just thought it should remain cohesive here!

marcelsavegnago
marcelsavegnago approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@marcelsavegnago marcelsavegnago left a comment

Choose a reason for hiding this comment

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

LGTM. It's really pretty. Congratulations.

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

@max3903
Copy link
Member

max3903 commented Oct 31, 2020

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 13.0-ocabot-merge-pr-602-by-max3903-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 94338c1 into OCA:13.0 Oct 31, 2020
@OCA-git-bot
Copy link
Contributor

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

@brian10048 brian10048 removed a link to an issue Jan 16, 2021
Migration to version 13.0 #354
Open
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@murtuzasaleh murtuzasaleh murtuzasaleh left review comments

@bodedra bodedra bodedra approved these changes

@marcelsavegnago marcelsavegnago marcelsavegnago approved these changes

@max3903 max3903 max3903 approved these changes

@nelsonramirezs nelsonramirezs Awaiting requested review from nelsonramirezs

@RLeeOSI RLeeOSI Awaiting requested review from RLeeOSI

Assignees

@brian10048 brian10048

Labels
approved merged 🎉 migration needs review ready to merge
Projects
None yet
Milestone
13.0
Development

Successfully merging this pull request may close these issues.