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

[14.0][ADD] shopfloor_reception_packaging_dimension #647

Merged
merged 7 commits into from
Nov 7, 2023

Conversation

TDu
Copy link
Member

@TDu TDu commented Jun 8, 2023

This will still need to be rebased after #559 has been merged.

ref.: rau-156

@OCA-git-bot
Copy link
Contributor

Hi @simahawk, @JuMiSanAr, @guewen, @sebalix, @mmequignon,
some modules you are maintaining are being modified, check this out!

@TDu
Copy link
Member Author

TDu commented Jul 7, 2023

rebased

@TDu
Copy link
Member Author

TDu commented Jul 11, 2023

The last 2 commits refactor the modules to allow extending the fields to update on a product.packaging.

@TDu TDu force-pushed the bsrau-156 branch 2 times, most recently from e84a932 to 07f86fc Compare July 25, 2023 13:35
@TDu
Copy link
Member Author

TDu commented Jul 25, 2023

Squashed and rebased

Copy link
Contributor

@mt-software-de mt-software-de left a comment

Choose a reason for hiding this comment

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

Wouldn't it be also good,
to have some button or a logic to create a packaging if none exists?
cc @jbaudoux

@jbaudoux
Copy link
Contributor

Wouldn't it be also good, to have some button or a logic to create a packaging if none exists? cc @jbaudoux

Not in this PR

Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

LG

@jbaudoux
Copy link
Contributor

@TDu Can you squash and rebase?

Copy link
Contributor

@mt-software-de mt-software-de left a comment

Choose a reason for hiding this comment

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

Some minor remarks

@TDu
Copy link
Member Author

TDu commented Jul 26, 2023

Squashed and rebased.

shopfloor/actions/message.py Outdated Show resolved Hide resolved
@@ -770,6 +770,10 @@ def _align_product_uom_qties(self, move):
for line in lines:
line.product_uom_qty = line.qty_done + remaining_todo

def _before_response_for_set_quantity(self, picking, line):
Copy link
Contributor

Choose a reason for hiding this comment

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

the name of this method seems wrong... Is not "before" is actually replacing the direct call to the other method 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually inserts a new front end state before the set quantity state if needed. Names can always be improved...

Copy link
Contributor

Choose a reason for hiding this comment

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

why don't you call it then maybe _response_after_scan_line__assign_user ?

Copy link
Member Author

@TDu TDu Aug 2, 2023

Choose a reason for hiding this comment

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

well, it is also called after set_lot_confirm_action so I don't think it it really fits.

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I'd prefer something more explicit. The main issue here is that we are trying to change the behavior of something that does not take into account the possibility to handle dimensions.
We could assume - that being a WMS app - we can loosely support such extension.

What if we simply offer an explicit hook here (and everywhere else needed)?
Eg:

       if product.tracking not in ("lot", "serial") or (line.lot_id or line.lot_name):
            if self._requires_packaging_dimension(...):
                return self._response_for_set_packaging_dimension(...)
            return self._response_for_set_quantity(picking, line)

[....]

def _response_for_set_packaging_dimension(self, .....):
    raise NotImplementedError()

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure why this is better. It adds more coupling between module when not needed.

What is being done here is inserting a new backend state in a scenario before an existing one. When that new state has achieved its purpose (multiple conversation/interaction between the frontend and backend) the scenario will proceed as initially intended.

Now I still agree that the naming is not good. Because the insertion should not be done before generating the response for set_quantity (which is called for the first and subsequent access to the state) but before entering the state set_quantity.

So in that light I refactored the code renaming the function to _before_state__set_quantity.
It does not add coupling and can be used by other modules.

_logger.info("Add set packaging dimension option on reception scenario")
env = api.Environment(cr, SUPERUSER_ID, {})
scenario = env.ref("shopfloor_reception.scenario_reception")
options = json.loads(scenario.options_edit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
options = json.loads(scenario.options_edit)
options = scenario.options

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not 🤣

self.work.dimension_to_update = {}
return self.work.dimension_to_update

def _before_response_for_set_quantity(self, picking, line):
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 understand why you needed a new method to override instead of overriding _response_for_set_qty 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It seemed easier at the time. Looking at it now, because _response_for_set_quantity is called in many places in the scenario code and only in a few places we need to inject the packaging dimension check to return a specific response.

Copy link
Member

Choose a reason for hiding this comment

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

_response_for_set_quantity could be called from elsewhere, it's nice to be able to use this hook or not.
I would check if there's packaging available to set dimension where it's relevant, and return _response_for_set_packaging_dimension when we find some.

// Extend the reception scenario with :
// - the new patched template
// - the js code for the new state
const ReceptionPackageDimension = process_registry.extend("reception", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try this?

Vue.componen("reception", {
     "methods": {
          [....]
      }
})

Copy link
Member Author

Choose a reason for hiding this comment

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

2 reasons for this not to be working.
First one is in how we declare the Vue component for each scenario. At the moment those components are registered as Anonymous Component. But it can be fixed by adding the name key in the scenario component.

The 2nd one is at the time this file is being interpreted by the browser, the Vue component is not created yet, it is created later at the app creation

Copy link
Contributor

@simahawk simahawk Aug 2, 2023

Choose a reason for hiding this comment

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

Yeah, you are right. I had an eureka moment some days ago in the shower (and forgot to reply here): the main issue is that scenario components are registered as local components on the root app. That's it.
We should refactor this

components: APP_COMPONENTS,
(I mean, drop it, and register such components in the std way).
Go ahead like this for now. Maybe add a TODO then we'll see.

@@ -770,6 +770,10 @@ def _align_product_uom_qties(self, move):
for line in lines:
line.product_uom_qty = line.qty_done + remaining_todo

def _before_response_for_set_quantity(self, picking, line):
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't you call it then maybe _response_after_scan_line__assign_user ?

Copy link
Contributor

@mt-software-de mt-software-de left a comment

Choose a reason for hiding this comment

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

LGTM - except the naming of _before_response_for_set_quantity

After testing there must be a change for the js overwrite of the reception states

_logger.info("Add set packaging dimension option on reception scenario")
env = api.Environment(cr, SUPERUSER_ID, {})
scenario = env.ref("shopfloor_reception.scenario_reception")
options = json.loads(scenario.options_edit)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not 🤣

@@ -770,6 +770,10 @@ def _align_product_uom_qties(self, move):
for line in lines:
line.product_uom_qty = line.qty_done + remaining_todo

def _before_response_for_set_quantity(self, picking, line):
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I'd prefer something more explicit. The main issue here is that we are trying to change the behavior of something that does not take into account the possibility to handle dimensions.
We could assume - that being a WMS app - we can loosely support such extension.

What if we simply offer an explicit hook here (and everywhere else needed)?
Eg:

       if product.tracking not in ("lot", "serial") or (line.lot_id or line.lot_name):
            if self._requires_packaging_dimension(...):
                return self._response_for_set_packaging_dimension(...)
            return self._response_for_set_quantity(picking, line)

[....]

def _response_for_set_packaging_dimension(self, .....):
    raise NotImplementedError()

WDYT?

Copy link
Contributor

@mt-software-de mt-software-de left a comment

Choose a reason for hiding this comment

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

Javascript tries to send max_weight but python is looking for weight.

@TDu
Copy link
Member Author

TDu commented Oct 6, 2023

I have answered or/and fixed all comments.
Rebased as well.

@jbaudoux
Copy link
Contributor

@simahawk Are we good now with this one? Can you update your review?
@mt-software-de Your comments have been tackled, can you update your review?

Copy link
Contributor

@mt-software-de mt-software-de left a comment

Choose a reason for hiding this comment

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

LGTM

@jbaudoux
Copy link
Contributor

@TDu Can you squash ?

@simahawk
Copy link
Contributor

simahawk commented Nov 7, 2023

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-647-by-simahawk-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e78312f into OCA:14.0 Nov 7, 2023
6 of 8 checks passed
@OCA-git-bot
Copy link
Contributor

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

6 participants