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

[16.0][MIG] sale_delivery_state #2280

Closed
wants to merge 25 commits into from

Conversation

atchuthan
Copy link
Member

@atchuthan
Copy link
Member Author

@OCA/community-maintainers Test fails due to mock python package missing at https:/OCA/sale-workflow/actions/runs/3540792580/jobs/5944271297

  File "/__w/sale-workflow/sale-workflow/sale_delivery_state/tests/__init__.py", line 1, in <module>
    from . import test_delivery_state
  File "/__w/sale-workflow/sale-workflow/sale_delivery_state/tests/test_delivery_state.py", line 6, in <module>
    import mock
ModuleNotFoundError: No module named 'mock'

@rousseldenis
Copy link
Sponsor Contributor

/ocabot migration sale_delivery_state

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Dec 7, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Dec 7, 2022
97 tasks
@RodrigoBM
Copy link

@rousseldenis you think it is a good idea to remove the logic associated with force_delivery_state?

I think that if I have decided to set the status of a sale to delivered as done manually, this status should not be recalculated if the quantity of any line is changed.

But if this change is correct, the action_unforce_delivery_state method is not needed, as the delivery_state field is now selectable. And the name of the function action_unforce_delivery_state should be changed to for example action_recalculate_delivery_state

@rousseldenis
Copy link
Sponsor Contributor

@rousseldenis you think it is a good idea to remove the logic associated with force_delivery_state?

I think that if I have decided to set the status of a sale to delivered as done manually, this status should not be recalculated if the quantity of any line is changed.

But if this change is correct, the action_unforce_delivery_state method is not needed, as the delivery_state field is now selectable. And the name of the function action_unforce_delivery_state should be changed to for example action_recalculate_delivery_state

As I said:

But maybe this is something unwanted as we really want to force it (even if qty_delivered on lines change)

So, maybe it's a good idea to keep the force field with current changes too.

@RodrigoBM
Copy link

RodrigoBM commented Dec 30, 2022

I think that both solutions can be correct

@rousseldenis you think it is a good idea to remove the logic associated with force_delivery_state?
I think that if I have decided to set the status of a sale to delivered as done manually, this status should not be recalculated if the quantity of any line is changed.
But if this change is correct, the action_unforce_delivery_state method is not needed, as the delivery_state field is now selectable. And the name of the function action_unforce_delivery_state should be changed to for example action_recalculate_delivery_state

As I said:

But maybe this is something unwanted as we really want to force it (even if qty_delivered on lines change)

So, maybe it's a good idea to keep the force field with current changes too.

But considering that this is a migration issue, we should keep the development logic. What do you think? @atchuthan @dreispt

@atchuthan
Copy link
Member Author

I think that both solutions can be correct

@rousseldenis you think it is a good idea to remove the logic associated with force_delivery_state?
I think that if I have decided to set the status of a sale to delivered as done manually, this status should not be recalculated if the quantity of any line is changed.
But if this change is correct, the action_unforce_delivery_state method is not needed, as the delivery_state field is now selectable. And the name of the function action_unforce_delivery_state should be changed to for example action_recalculate_delivery_state

As I said:

But maybe this is something unwanted as we really want to force it (even if qty_delivered on lines change)

So, maybe it's a good idea to keep the force field with current changes too.

But considering that this is a migration issue, we should keep the development logic. What do you think? @atchuthan @dreispt

so, do we revert the code to use force field as it was in v15? @rousseldenis @RodrigoBM

@RodrigoBM
Copy link

I think that both solutions can be correct

@rousseldenis you think it is a good idea to remove the logic associated with force_delivery_state?
I think that if I have decided to set the status of a sale to delivered as done manually, this status should not be recalculated if the quantity of any line is changed.
But if this change is correct, the action_unforce_delivery_state method is not needed, as the delivery_state field is now selectable. And the name of the function action_unforce_delivery_state should be changed to for example action_recalculate_delivery_state

As I said:

But maybe this is something unwanted as we really want to force it (even if qty_delivered on lines change)

So, maybe it's a good idea to keep the force field with current changes too.

But considering that this is a migration issue, we should keep the development logic. What do you think? @atchuthan @dreispt

so, do we revert the code to use force field as it was in v15? @rousseldenis @RodrigoBM

In my opinion, I think so.

@RodrigoBM
Copy link

@atchuthan, can you revert the last change?

@atchuthan
Copy link
Member Author

@atchuthan, can you revert the last change?

@RodrigoBM reverted. Please review

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

Copy link

@RodrigoBM RodrigoBM left a comment

Choose a reason for hiding this comment

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

Checking version 16, in the base module sale_stock it adds the delivery_status field which is the same logic as the delivery_state field in this module.
I think this module is no more necessary. And create a new module called sale_force_delivery for example, to add the force status logic for the new delivery_status base field.
cc: @atchuthan @rousseldenis @dreispt

Copy link

@RodrigoBM RodrigoBM left a comment

Choose a reason for hiding this comment

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

@atchuthan
Copy link
Member Author

Checking version 16, in the base module sale_stock it adds the delivery_status field which is the same logic as the delivery_state field in this module. I think this module is no more necessary. And create a new module called sale_force_delivery for example, to add the force status logic for the new delivery_status base field. cc: @atchuthan @rousseldenis @dreispt

I am okay with removing this module in favor of odoo's already field, and we will remove the dependency to sale_automatic_workflow too. Please confirm whether I can proceed or not?

@atchuthan
Copy link
Member Author

Closing this PR as this module is not needed for v16

@atchuthan atchuthan closed this Jan 10, 2023
@atchuthan atchuthan deleted the 16.0-mig-sale_delivery_state branch February 24, 2023 07:43
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.