[16.0][mig] customer_wallet_(account-portal-pos)#397
[16.0][mig] customer_wallet_(account-portal-pos)#397legalsylvain wants to merge 111 commits intocoopiteasy:16.0from
Conversation
|
Hi @carmenbianca, |
27b3f7d to
92d3593
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 16.0 #397 +/- ##
===========================================
- Coverage 94.17% 81.54% -12.64%
===========================================
Files 34 205 +171
Lines 584 2649 +2065
Branches 56 261 +205
===========================================
+ Hits 550 2160 +1610
- Misses 27 446 +419
- Partials 7 43 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c5985a0 to
78a85f3
Compare
|
Ready for review. Full description of account_wallet module :Full description of account_wallet_pos module :Full description of account_wallet_portal module : |
a6bffd8 to
5299fbb
Compare
Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
….config Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
…t payment method Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
…stead of settings Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
…ines This does not yet live-update in the POS. The window needs to be reloaded to see the updated balance. Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
… balance Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be> Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
…point_of_sale module
…etc...) when displaying partners with wallet account
…he customer wallet balance - Add a dedicated wizard and menu entry to see all the wallets. - Refactor & Improve computation mechanism. - Fix incorrect _customer_wallet_depends() if point_of_sale is installed - Total refactor of the portal part. Display credit and debit + fix: display pos pending moves.
… main compute function
…he core customer_wallet module doesn't work, when point of sale is installed
…allet minimum amount changed
ea8653c to
37b4cc9
Compare
huguesdk
left a comment
There was a problem hiding this comment.
as requested, here is my in-progress review. i will continue reviewing it afterwards.
but first of all: great work, thanks! i tested it functionally, and it works like a charm.
There was a problem hiding this comment.
please don’t modify this file directly: it will be updated automatically on merge.
There was a problem hiding this comment.
I can't ! It's done automatically by pre-commit execution AFAIK.
There was a problem hiding this comment.
pre-commit generates the modules’ README.rst and index.html files, but this one is generated by oca-gen-addons-table. i think the changes here are a result of search and replace, and they might cause merge conflicts.
| @@ -8,19 +8,24 @@ | |||
| "version": "16.0.1.0.1", | |||
| "category": "Accounting & Finance", | |||
There was a problem hiding this comment.
category names have another structure now.
| "category": "Accounting & Finance", | |
| "category": "Accounting/Accounting", |
| "views/product_template_views.xml", | ||
| "views/res_config_settings_views.xml", | ||
| "views/res_partner_views.xml", | ||
| "wizards/res_config_settings_views.xml", |
There was a problem hiding this comment.
this is indeed a TransientModel, but usually, it is still put in the views directory (and its model in models).
There was a problem hiding this comment.
creating a demo account (and journals) directly like this in xml doesn’t work if a new database is initialized with this module directly: at the end of the init, the account and journals are deleted when the chart of account is created in the post_init_hook of the account module. the cooperator module works around this by initializing the data in another way (see this function that is called from several places (data.xml and account.chart.template._load()) (the demo data is also initialized explicitly from this xml file to support the rare case where demo data is loaded afterwards).
because of this, if a database is initialized with this module directly, the tests will fail. however, the tests on the ci run anyway because the database is first initialized with all the modules this module depends on and the tests are run in a second step, when installing this module (actually, all the modules of the repository).
note that it is not that important (as it is only demo data, and it was already the case in 12) and can be added in the roadmap.
There was a problem hiding this comment.
the name of the module is still “Account Customer Wallet”, while the technical name has changed. should the name be adapted also?
| self.assertEqual(self.partner.customer_wallet_balance, 100) | ||
| self.assertEqual(self.other_partner_1.customer_wallet_balance, 0) | ||
|
|
||
| self._redistribute(30.0, [(self.other_partner_1, 30)]) |
There was a problem hiding this comment.
i tried to perform this operation from the web ui, and it doesn’t work. as soon as i click on “redistribute”, the amount field (which i set at 30) resets to 100 and the user error dialog pops up saying “The amount to be redistributed (100.0) is different from the sum of the amounts (30.0).”. it seems that the amount field gets recomputed. maybe instead of using a compute function, use an onchange on partner_id?
| <field name="arch" type="xml"> | ||
|
|
||
| <button name="action_view_partner_invoices" position="after"> | ||
| <button |
| <button | ||
| type="object" | ||
| name="action_view_customer_wallet_details" | ||
| string="Details" | ||
| class="btn-secondary" | ||
| attrs="{'invisible': [('has_customer_wallet', '=', False)]}" | ||
| /> | ||
| <button | ||
| type="object" | ||
| name="action_view_customer_wallet_redistribute" | ||
| string="Redistribute" | ||
| class="btn-secondary" | ||
| attrs="{'invisible': [('customer_wallet_balance', '<=', 0.0)]}" | ||
| /> |
There was a problem hiding this comment.
these buttons are a great addition! however, they take quite some space in the list and cannot be hidden. moreover, the redistribute action is only available from this list. adding it as a form action on the partner would be great.
| <field name="context">{ | ||
| 'search_default_company_and_individual': 1, | ||
| 'search_default_customer_wallet_balance_nonzero': 1, | ||
| 'active_test': False, |
| </record> | ||
|
|
||
| <record id="action_res_partner_wallet" model="ir.actions.act_window"> | ||
| <field name="name">Customer Wallets</field> |
There was a problem hiding this comment.
this act_window is a nice addition too! the only strange thing about it is that it has a “new” button, which allows to create a partner. i think that’s confusing, as it should mean “new customer wallet” (even if that doesn’t make much sense). i think it’s better to prevent to create new records there.
huguesdk
left a comment
There was a problem hiding this comment.
again, great work, @legalsylvain! here is the rest of my remarks.
question: why were the modules renamed? the convention in odoo seems to be {application_name}_{feature_name} so the previous names made more sense i think, as customer_wallet is not an application.
also, please squash the rename commits, as there have been some wrongly renamed values corrected afterwards.
thanks a lot for this work!
| <div | ||
| class="alert alert-warning text-center" | ||
| attrs="{'invisible': ['|', ('is_customer_wallet_journal', '=', True), ('customer_wallet_balance', '<=', '0')]}" | ||
| role="alert" | ||
| > | ||
| You can select the customer account journal for this customer. | ||
| </div> |
There was a problem hiding this comment.
great addition! the funny thing is that you can pay an invoice containing the customer wallet product with the amount that this invoice represents, resulting in a null operation, without actually having transferred any money.
| detail = fields.Html(compute="_compute_detail_html", sanitize=False) | ||
|
|
||
| @api.depends("partner_id") | ||
| def _compute_detail_html(self): |
There was a problem hiding this comment.
as explained earlier, i think it is better to call a method on the partner to compute the details here, instead of using a computed field on the partner that will be computed over and over mostly uselessly.
There was a problem hiding this comment.
typo in the filename: should be redistribute instead of reditribute (s missing)
| type="object" | ||
| class="oe_highlight" | ||
| /> | ||
| or |
There was a problem hiding this comment.
this "or" should be removed: it is implied and it displays badly (vertical alignment is bad).
| ) | ||
|
|
||
| amount = fields.Monetary( | ||
| compute="_compute_amount", |
There was a problem hiding this comment.
should probably use an onchange (and maybe a default also?) instead of compute, as the amount is reset when clicking the “redistribute” button.
| is_balance_above_minimum(client, wallet_amount) { | ||
| return ( | ||
| client.customer_wallet_balance - wallet_amount <= | ||
| this.env.pos.config.minimum_wallet_amount - 0.00001 |
There was a problem hiding this comment.
this seems hacky (hardcoding a delta value like this). isn’t there a cleaner way?
| for (var i = 0; i < this.payment_methods_from_config.length; i++) { | ||
| if (this.payment_methods_from_config[i].is_customer_wallet_method) { | ||
| return this.payment_methods_from_config[i]; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
| for (var i = 0; i < this.payment_methods_from_config.length; i++) { | |
| if (this.payment_methods_from_config[i].is_customer_wallet_method) { | |
| return this.payment_methods_from_config[i]; | |
| } | |
| } | |
| return null; | |
| return this.payment_methods_from_config.find((c) => c.is_customer_wallet_method) || null; |
| var wallet_products = []; | ||
| for (const value of Object.values(this.env.pos.db.product_by_id)) { | ||
| if (value.is_customer_wallet_product) { | ||
| wallet_products.push(value); | ||
| } | ||
| } | ||
| return wallet_products; |
There was a problem hiding this comment.
| var wallet_products = []; | |
| for (const value of Object.values(this.env.pos.db.product_by_id)) { | |
| if (value.is_customer_wallet_product) { | |
| wallet_products.push(value); | |
| } | |
| } | |
| return wallet_products; | |
| return Object.values(this.env.pos.db.product_by_id).filter((p) => p.is_customer_wallet_product); |
| var order = this.currentOrder; | ||
| var method = this.find_customer_wallet_payment_method(); | ||
| var wallet_amount = 0; | ||
| var lines_qty = 0; | ||
| order.paymentlines.forEach((item) => { | ||
| if (item.payment_method === method) { | ||
| wallet_amount += item.amount; | ||
| lines_qty += 1; | ||
| } | ||
| }); | ||
| return [wallet_amount, lines_qty]; |
There was a problem hiding this comment.
| var order = this.currentOrder; | |
| var method = this.find_customer_wallet_payment_method(); | |
| var wallet_amount = 0; | |
| var lines_qty = 0; | |
| order.paymentlines.forEach((item) => { | |
| if (item.payment_method === method) { | |
| wallet_amount += item.amount; | |
| lines_qty += 1; | |
| } | |
| }); | |
| return [wallet_amount, lines_qty]; | |
| const method = this.find_customer_wallet_payment_method(); | |
| const lines = this.currentOrder.paymentlines.filter((line) => line.payment_method === method); | |
| const wallet_amount = lines.reduce((sum, line) => sum + line.amount, 0); | |
| return [wallet_amount, lines.length]; |
| var order = this.currentOrder; | ||
| var wallet_product_ids = []; | ||
| var wallet_products = this.find_customer_wallet_products(); | ||
| wallet_products.forEach(function (product) { | ||
| wallet_product_ids.push(product.id); | ||
| }); | ||
| var wallet_amount = 0; | ||
| var lines_qty = 0; | ||
|
|
||
| order.orderlines.forEach((orderline) => { | ||
| if (wallet_product_ids.includes(orderline.product.id)) { | ||
| wallet_amount += orderline.get_price_without_tax(); | ||
| lines_qty += 1; | ||
| } | ||
| }); | ||
|
|
||
| return [wallet_amount, lines_qty]; |
There was a problem hiding this comment.
| var order = this.currentOrder; | |
| var wallet_product_ids = []; | |
| var wallet_products = this.find_customer_wallet_products(); | |
| wallet_products.forEach(function (product) { | |
| wallet_product_ids.push(product.id); | |
| }); | |
| var wallet_amount = 0; | |
| var lines_qty = 0; | |
| order.orderlines.forEach((orderline) => { | |
| if (wallet_product_ids.includes(orderline.product.id)) { | |
| wallet_amount += orderline.get_price_without_tax(); | |
| lines_qty += 1; | |
| } | |
| }); | |
| return [wallet_amount, lines_qty]; | |
| const wallet_products_ids = this.find_customer_wallet_products().map(p => p.id); | |
| const lines = this.currentOrder.orderlines.filter((line) => wallet_product_ids.includes(orderline.product.id)); | |
| const wallet_amount = lines.reduce((sum, line) => sum + line.get_price_without_tax(), 0); | |
| return [wallet_amount, lines.length]; |
Description
Fixes
websitemodule.refactoring
New features
account.payment.register: add a banner on payment form, to mention that it is possible to use Wallet Journal.res.partner: new tree view to see all customer wallet amounts.res.partner: add the wallet amount in the top of partner form viewres.partner: have the possibility to see the detail of all the moves that justify the current wallet amount. + same in portal.Doc
Checklist before approval