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

[13.0] [MIG] fieldservice_sale #595

Merged
merged 72 commits into from
Mar 9, 2022
Merged

Conversation

brian10048
Copy link
Contributor

@brian10048 brian10048 commented Jul 18, 2020

Migration of fieldservice_sale to 13.0

#354

Depends on #598

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

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

@brian10048 brian10048 force-pushed the 13.0-mig-fsm-sale branch 5 times, most recently from 67fc2f3 to 8d77aa2 Compare July 21, 2020 19:03
@brian10048 brian10048 mentioned this pull request Aug 2, 2020
Copy link
Contributor

@RLeeOSI RLeeOSI left a comment

Choose a reason for hiding this comment

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

FSM territory was moved to base_territory, fixes error on res.config.settings.execute() with too many positional arguments

@brian10048
Copy link
Contributor Author

@RLeeOSI Thank you!

@bodedra
Copy link
Member

bodedra commented Oct 22, 2020

@brian10048 Would you please run "pre-commit run -a" # to run black, isort and prettier (ignore pylint errors at this stage)

@brian10048 brian10048 force-pushed the 13.0-mig-fsm-sale branch 2 times, most recently from 5f603f9 to 8839733 Compare October 22, 2020 20:28
@brian10048
Copy link
Contributor Author

@brian10048 Would you please run "pre-commit run -a" # to run black, isort and prettier (ignore pylint errors at this stage)

@bodedra Done, but will still need #598 before the build is green here


* Serpent Consulting Services Pvt. Ltd. <[email protected]>
* Brian McMaster <[email protected]>
* Rapha??l Reverdy <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

there is an encoding issue

there is also issues in po files.

@osi-scampbell
Copy link
Contributor

@brian10048 Hey Brian, the migration looks really good! I found a small bug in v12 that I have fixed here #668. The changes are small and easy and I was hoping you could implement these in this PR so we don't have this bug in v13. Thanks!

Copy link
Contributor

@hparfr hparfr left a comment

Choose a reason for hiding this comment

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

I may be miss something, but I think the logic about invoicing is not aligned with other parts of odoo (like sale_stock).

"""Test sale order 3 flow from sale to invoice.
- An FSM order should be created for each Sale Order Line.
- The FSM Order should update qty_delivered when completed.
- An Invoice linked to each FSM Order should be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get this.

All fsm orders shoudn't be in the same invoice ?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Asked @osi-scampbell about this one. Some SOs will trigger creation of multiple FSOs. One (or more) of those FSOs can include labor that wasn't included on the original SO. Therefore, it creates its own invoice for those lines not included on the original SO. This is an intended feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is an intended feature. However, I don't prefer it for my usage.

Does anyone have suggestions for how we can accomplish both ways? Maybe a setting in the configuration panel that will either split invoices or keep them as one?

Copy link
Contributor

Choose a reason for hiding this comment

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

My thoughts exactly. It would be nice to add a feature that allows a choice between the existing functionality (each FSO generates an invoice) and consolidating invoices for all FSOs created from the same SO. But that's not really part of migration, it's a new feature. I'm not familiar with OCA best practices in this case, but IMO it's best to leave new features separate from the migration PR

Copy link
Member

Choose a reason for hiding this comment

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

Yes agreed @RLeeOSI New feature will not be part of migration. To introduce new feature, create a issue, represent blueprint and submit PR.

Copy link
Contributor

@hparfr hparfr Nov 19, 2020

Choose a reason for hiding this comment

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

One (or more) of those FSOs can include labor that wasn't included on the original SO. Therefore, it creates its own invoice for those lines not included on the original SO. This is an intended feature

I think it can be implemented with existing modules:
You can add sale order line with a product.type=service (labor) automatically when adding a product.type=fsm thanks to product-pack ( https://github.com/oca/product-pack ) or with more advanced settings ( https://github.com/akretion/sale-configurator ).
You can then track the progression of service with timesheets. It will be more aligned with other parts of odoo (stock, mrp).

Even if this proposition do not work for your use case, are you really sure this feature need to be included in fieldservice_sale and not in his own module ?
May be I'm wrong, but from point of view this feature is too specific to be included in fieldservice_sale. It can be easily extracted in another module, and letting it in fieldservice_sale adds complexity like now during a migration.

But that's not really part of migration, it's a new feature.

Let's say it's part of a migration cleanup/refactoring. Doing it now is may be the best moment to introduce breaking change like dependency graph and modules organization.

Copy link
Member

Choose a reason for hiding this comment

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

@hparfr I am good with migration cleanup/refactoring. But can you provide it with migration script and unit test?

Copy link
Contributor

@hparfr hparfr left a comment

Choose a reason for hiding this comment

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

LGTM !

Thanks

@dreispt dreispt requested a review from RLeeOSI May 8, 2021 21:54
@brian10048
Copy link
Contributor Author

@bodedra @RLeeOSI are either of you available to review this migration?

@RLeeOSI
Copy link
Contributor

RLeeOSI commented Jul 3, 2021

@brian10048 Was the invoice creation part completely removed? It looks like now there will be no link between the FSO and the invoice. Has that been moved to another module?

I recommend adding another selection field to product.template similar to field_service_tracking, that allows the user to choose whether the SO generates a single invoice, or one invoice per FSO.

@Chanakya-OSI
Copy link
Contributor

@brian10048 I want to add Migration Script in the fieldservice_sale module in V13 so can you please add me as a collaborator? Thanks

@brian10048
Copy link
Contributor Author

@Chanakya-OSI You should be able to submit a PR to my branch here. If everything looks good we can then merge this PR with your commits as well

@dreispt
Copy link
Member

dreispt commented Dec 4, 2021

/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-595-by-dreispt-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Dec 4, 2021
Signed-off-by dreispt
@brian10048
Copy link
Contributor Author

/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-595-by-brian10048-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit d30e607 into OCA:13.0 Mar 9, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 6b29dfe. 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.

Migration to version 13.0