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

12.0 rename beesdoo pos reporting #468

Merged
merged 4 commits into from
Nov 15, 2022

Conversation

victor-champonnois
Copy link
Member

@victor-champonnois victor-champonnois commented Oct 31, 2022

Description

fix #346

related PR

Odoo task (if applicable)

Checklist before approval

  • Tests are present (or not needed).
  • Credits/copyright have been changed correctly.
  • Change log snippet is present.
  • (If a new module) Moving this to OCA has been considered.

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Merging #468 (9f47909) into 12.0 (acee7a1) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             12.0     #468      +/-   ##
==========================================
+ Coverage   61.83%   61.93%   +0.09%     
==========================================
  Files         167      168       +1     
  Lines        5348     5356       +8     
  Branches      937      937              
==========================================
+ Hits         3307     3317      +10     
+ Misses       1902     1901       -1     
+ Partials      139      138       -1     
Impacted Files Coverage Δ
pos_order_count_store/models/__init__.py 100.00% <ø> (ø)
pos_order_count_store/models/res_partner.py 100.00% <100.00%> (ø)
pos_order_count_store/pre_init_hook.py 100.00% <100.00%> (ø)
beesdoo_shift/models/res_partner.py 43.58% <0.00%> (+1.70%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@robinkeunen robinkeunen left a comment

Choose a reason for hiding this comment

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

Looks good but could it be moved to the oca directly?

@victor-champonnois
Copy link
Member Author

@robinkeunen according to https://user-images.githubusercontent.com/17589077/149927982-58f1b329-9d15-48d3-b331-77741f7c2e87.png it should be moved to cie-pos, plans have changed ?

@robinkeunen
Copy link
Member

My point of view has changed now yes. Maybe @legalsylvain could tell us whether he thinks pos_order_count_store would be welcome on oca/pos ?

The description.rst :

Store pos_order_count to improve reporting.

Copy link

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi.

Thanks for the ping. I think that the module pos_order_count_store has its place under OCA umbrella. (generic and add features), as far as the "update" question is OK.

I think that if yes, you should add a pre-init-hook script to compute by SQL the stored field.
Otherwise, installing this module on existing databases (the GRAP one, for instance) will take hours for sure.

class ResPartner(models.Model):
_inherit = "res.partner"

pos_order_count = fields.Integer(store=True)

Choose a reason for hiding this comment

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

I fear that if we make an update all, the update of point_of_sale module drop the column, and then, the update of the pos_order_count_store module recompute it. Am I wrong ?

Choose a reason for hiding this comment

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

(I remember that I developed one time a module store = True -> store =False, and that was problematic.

Copy link
Member Author

@victor-champonnois victor-champonnois Nov 3, 2022

Choose a reason for hiding this comment

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

Thank you for your reply

I fear that if we make an update all, the update of point_of_sale module drop the column, and then, the update of the pos_order_count_store module recompute it. Am I wrong ?

It's not what I observe, when updating point_of_sale (and then pos_order_count_store), the field was never recomputed.

Noted for the SQL Query in pre-init hook, I'll do that !

@victor-champonnois
Copy link
Member Author

  • I added the pre_init_hook
  • @polchampion OK to test on coopeco-test and lesptitpots-test. To be tested : "Store pos_order_count to improve reporting." (check that the number of pos_order per partner is OK)

Copy link
Member

@robinkeunen robinkeunen left a comment

Choose a reason for hiding this comment

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

Thx for the pre-init hook 👍 Can you rebase, squash and move the module to oca/pos ?

@robinkeunen
Copy link
Member

/ocabot rebase

@github-grap-bot
Copy link
Contributor

Congratulations, PR rebased to 12.0.

@victor-champonnois
Copy link
Member Author

/ocabot rebase

@victor-champonnois
Copy link
Member Author

@robinkeunen LGTM, should be merged for next MEP, so that pos_counter_store is installed

@victor-champonnois
Copy link
Member Author

@robinkeunen can you approve the PR ?

@victor-champonnois victor-champonnois merged commit ffe4645 into 12.0 Nov 15, 2022
@victor-champonnois victor-champonnois deleted the 12.0-rename-beesdoo_pos_reporting branch November 15, 2022 14:41
@polchampion
Copy link
Collaborator

@victor-champonnois Sucessfully tested on coopeco-test : number of pos orders for one partner seems right (smart button on profile + pos order dynamic table view + filter on all orders for that partner.

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.

Rename beesdoo_pos_reporting to pos_order_count_store
6 participants