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] member_card #522

Merged
merged 32 commits into from
Jan 24, 2024
Merged

[16.0] [MIG] member_card #522

merged 32 commits into from
Jan 24, 2024

Conversation

victor-champonnois
Copy link
Member

@victor-champonnois victor-champonnois commented Aug 3, 2023

task : https://gestion.coopiteasy.be/web#id=10992&action=475&active_id=492&model=project.task&view_type=form&menu_id=

I changed the behavior when creating a new card in 6da9ef2

  • before, creating a new card would trigger the partner's _compute_bar_code method, which would set the card's barcode to the partner.
  • now the compute method is not triggered when creating a new card for unknown reasons. I then changed the behavior:
    • the barcode is assigned to the partner on the creation of the member card
    • the fields on the member card are readonly, to force the creation of a new member card (and avoid barcode not being modified on the partner when it is modified on the member card).
      @polchampion FYI (because it's a functional change) -> Pol approved

We looked at it with @robinkeunen and we didn't found a better solution, do @carmenbianca or @huguesdk or @remytms have an opinion ?

Note : set to draft because we need to know how to deal with eater member cards (komunigi decision)

@codecov-commenter
Copy link

Codecov Report

Merging #522 (6da9ef2) into 16.0 (5a5457c) will increase coverage by 2.18%.
The diff coverage is 92.68%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             16.0     #522      +/-   ##
==========================================
+ Coverage   64.17%   66.35%   +2.18%     
==========================================
  Files          34       43       +9     
  Lines        1485     1608     +123     
  Branches      207      211       +4     
==========================================
+ Hits          953     1067     +114     
- Misses        491      500       +9     
  Partials       41       41              
Files Changed Coverage Δ
member_card/wizard/member_card_wizards.py 81.81% <81.81%> (ø)
member_card/models/membercard.py 87.50% <87.50%> (ø)
member_card/__init__.py 100.00% <100.00%> (ø)
member_card/models/__init__.py 100.00% <100.00%> (ø)
member_card/models/partner.py 100.00% <100.00%> (ø)
member_card/models/res_company.py 100.00% <100.00%> (ø)
member_card/tests/__init__.py 100.00% <100.00%> (ø)
member_card/tests/test_member_card.py 100.00% <100.00%> (ø)
member_card/wizard/__init__.py 100.00% <100.00%> (ø)

@victor-champonnois victor-champonnois marked this pull request as draft August 4, 2023 15:02
Copy link
Collaborator

@carmenbianca carmenbianca left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but some criticisms.

Can we try to make the compute method work?

member_card/demo/cooperators.xml Outdated Show resolved Hide resolved
member_card/report/member_card_template.xml Outdated Show resolved Hide resolved
member_card/models/partner.py Outdated Show resolved Hide resolved
member_card/views/member_card.xml Outdated Show resolved Hide resolved
@victor-champonnois
Copy link
Member Author

Note : we spent some time investigating with Hugues on how to make the compute method work. It's likely to be a problem related to multi-company, and the fact that the original barcode field (in odoo/base) is company_dependent.
If we use a "new" barcode field (with another name) that is not overiding the original barcode field it works well.
We tried adding @api.depends_context('company') on the compute method, and company_dependent=False on the parameters. But none works.
To be continued...

@victor-champonnois
Copy link
Member Author

@carmenbianca @huguesdk How do we move on with this ? Should we give up trying to make the compute method work and do it another way ?

@huguesdk
Copy link
Member

i think we should first make some tests with a minimal module to understand how we’re supposed to do it (i did something similar to test out how to do company-dependent related fields for cooperator).

@carmenbianca
Copy link
Collaborator

I'm making some progress on this, but I am (partially) blocked by odoo/odoo#137141

@carmenbianca
Copy link
Collaborator

Reverted the workaround, added a fix. Setting company_dependent=False was enough, in spite of your prior research. Not sure how that went wrong.

Awaiting review.

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.

We're getting there

member_card/demo/cooperators.xml Outdated Show resolved Hide resolved
member_card/models/partner.py Outdated Show resolved Hide resolved
@robinkeunen robinkeunen marked this pull request as ready for review December 20, 2023 15:51
robinkeunen and others added 7 commits December 20, 2023 16:51
into:
- product_main_supplier
- sale_suggested_price
- sale_adapt_price_wizard

[REF] split beesdoo_product -> product_main_supplier

[FIX] sale_suggested_price dep on product_main_supplier

[ADD] module sale_edit_price_wizard

[REF] rename sale_edit_price_wizard ->  sale_adapt_price_wizard

[REF] move rounding_method to sale_suggested_price

[REF] refactor following PR 320

[REF] search product_code -> product_main_seller

[FIX] bug adapt_sales_price

[IMP] use float_compare to compare prices

[REF] move list_price_write_date to sale_adapt_price_wizard

[FIX] beesdoo_product: main_seller_id search

[FIX] run precommit

fix
This reverts a change made in 9696118,
PR #289.

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@polchampion
Copy link
Collaborator

polchampion commented Jan 4, 2024

Functional test on 04/01/2023 and 12/01/2024 on 16-test-member-card:
@victor-champonnois @robinkeunen I improved the list of features a tad:

  • Adds a member_card_template view and print option on partner
  • Adds a "Member card" tab on the partner with a button to create a new member card
  • The partner's card and barcode history is visible in the member card tab
  • Creating a card generates a barcode, witch is then displayed on the member card
  • The "Force Barcode" option allows to set a specific barcode instead
  • A partner's barcode is computed from the last active member card
  • Adds a field member_card_logo on the company allowing to upload an image
  • The card template displays the member_card_logo image
  • A boolean "Print Member card?" allows to flag partners for whom you need to print new cards.
  • The wizards "Request member card printing" and "Set member card as printed" allow to mass check and uncheck the "Print Member Card?" flag.
  • If the point of sale is installed, the generated barcode matches customer pattern rule.

@robinkeunen
Copy link
Member

@carmenbianca ready for another review

@polchampion
Copy link
Collaborator

@robinkeunen functional tests ok, I updated my previous post!

Copy link
Member

@huguesdk huguesdk left a comment

Choose a reason for hiding this comment

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

lgtm. i squashed the migration-related commits together.

@robinkeunen
Copy link
Member

/ocabot merge nobump

@github-grap-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-522-by-robinkeunen-bump-nobump, awaiting test results.

@github-grap-bot github-grap-bot merged commit 76bae8f into 16.0 Jan 24, 2024
2 checks passed
@github-grap-bot github-grap-bot deleted the 16.0-mig-member_card branch January 24, 2024 10:39
@github-grap-bot
Copy link
Contributor

Congratulations, your PR was merged at 2579391. Thanks a lot for contributing to beescoop. ❤️

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.

7 participants