Skip to content

Conversation

wardm95
Copy link
Contributor

@wardm95 wardm95 commented Jun 22, 2017

Description of the issue/feature this PR addresses:

Current behavior before PR:

  • Open a POS session
    • All products from cache are loaded into POS session
  • Create a new product or change an existing product.
  • Reload POS session (with F5 or Close/Resume)
    • All products from cache are loaded into POS session again
    • New or changed products are not available in POS session

Desired behavior after PR is merged:

  • Open a POS session
    • All products from cache are loaded into POS session
  • Create a new product or change an existing product.
  • Reload POS session (with F5 or Close/Resume)
    • All new or changed products are loaded in the cache
    • All products from cache are loaded into POS session again
    • New or changed products are now available in POS session

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@Yenthe666
Copy link
Collaborator

Hey @jorenvo could you look at this PR when you've got time? One of my team members tried to improve the pos cache module. :-)

@jorenvo jorenvo self-assigned this Jun 22, 2017
@Yenthe666
Copy link
Collaborator

As for the CLA that is failing in the Runbot.. this is handled with PR #17774
The e-mail was incorrect on the company CLA.

@jorenvo
Copy link
Contributor

jorenvo commented Jun 23, 2017

@mart-e I'm not sure what to do with this. I think the idea is ok, but I'm not sure worth the risk merging this in stable. What do you think?

@Yenthe666
Copy link
Collaborator

Yenthe666 commented Jun 23, 2017

I think technically it doesn't break any stable guides but I was wondering the same. Perhaps it also cleaner to change the api.one to an api.multi with a self.ensure_one?

P.s: Thank you for the very fast response jorenvo!

@mart-e
Copy link
Contributor

mart-e commented Jun 23, 2017

Sorry but indeed, this is a change of behaviour that is not without risk and I would rather not see an improvement in 10.0.
Doesn't this patch risks to put the modified products in duplicate in the cache?
If you want to add new products, you should use the create_date. If you want to update modified products, you should check to avoid duplicates.

And yes, api.multi with self.ensure_one() is preferred if you have only one record.

Also, make the method private (starting with an underscore).

@Yenthe666
Copy link
Collaborator

Yenthe666 commented Jun 23, 2017

@wardm95 let us make a new one against master with the suggestions from Martin.

@Yenthe666 Yenthe666 closed this Jun 23, 2017
@wardm95
Copy link
Contributor Author

wardm95 commented Jun 23, 2017

@Yenthe666 great idea :)

@jorenvo
Copy link
Contributor

jorenvo commented Jun 23, 2017

@mart-e okay, I agree. Thanks!

@wardm95 apart from the duplicate products issue it would also be nice to keep code duplication as minimal as possible. add_new_products_to_cache is an almost identical copy of refresh_cache. It also doesn't include the domain returned by get_product_domain so it could potentially include new products that shouldn't be included. All of this can be avoided by instead adding an extra parameter to refresh_cache, e.g. update=False, and using that to figure out what to do. Ping me on the new PR and I'll gladly take a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants