Tryton - Issues

 

Issue7890

Title Additional informations on Payment Group
Priority feature Status resolved
Superseder Nosy List ced, reviewbot, roundup-bot, semarie
Type feature request Components account_payment
Assigned To semarie Keywords review
Reviews 305581002
View: 305581002

Created on 2018-11-28.10:59:37 by semarie, last changed by roundup-bot.

Messages
New changeset 4942a9d07324 by Cédric Krier in branch 'default':
Remove junk from payment group view when applying patches
https://hg.tryton.org/tryton-env/rev/4942a9d07324
New changeset a80463f53c97 by Cédric Krier in branch 'default':
Remove junk from payment group view when applying patches
https://hg.tryton.org/modules/account_payment/rev/a80463f53c97
New changeset e9c6fd6a373b by Cédric Krier in branch 'default':
Add aggregated information on Payment groups
https://hg.tryton.org/tryton-env/rev/e9c6fd6a373b
New changeset 985fe4c47073 by Sebastien Marie in branch 'default':
Add aggregated information on Payment groups
https://hg.tryton.org/modules/account_payment/rev/985fe4c47073
review305581002 updated at https://codereview.tryton.org/305581002/#ps293871002
review305581002 updated at https://codereview.tryton.org/305581002/#ps327211002
review305581002 updated at https://codereview.tryton.org/305581002/#ps293731002
review305581002 updated at https://codereview.tryton.org/305581002/#ps325371002
review305581002 updated at https://codereview.tryton.org/305581002/#ps317401002
msg45638 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-12-30.23:41:58
You do not need to test backend to use filter_, python-sql manage that.
Instead of setting empty dict on result, you could just use a
defaultdict like this:

    result = defaultdict(defaultdict)

You could loop over the cursor dict instead of testing each name.

For payment_complete the SQL expression could use EVERY which is
standard (but we need to add a fallback in python-sql like
https://blog.jooq.org/2014/12/18/a-true-sql-gem-you-didnt-know-yet-the-every-aggregate-function/)

On 2018-12-25 10:20, Sebastien Marie wrote:
> and a question: in get_payment_aggregated() I am using:
>     cls(group_id).currency_digits
> to retreive the number of digits to quantize the amounts.
> 
> Should the method uses @fields.depends('currency_digits') decorator ?
> 
> I am unsure. The fields have the dependency to 'currency_digits' at class level, and the method is used for compute the value of the fields.

No depends can not be used on class method.
Indeed you need to round only for SQLite backend and it is better to use
groups instances for that to take advantage of the background cache and batch.
You can find an example in Invoice.get_amount

I do not think scenario_account_payment_invoice.rst need to be updated.
Having one scenario that test the new function fields is enough.
msg45503 (view) Author: [hidden] (semarie) Date: 2018-12-25.10:20:50
and a question: in get_payment_aggregated() I am using:
    cls(group_id).currency_digits
to retreive the number of digits to quantize the amounts.

Should the method uses @fields.depends('currency_digits') decorator ?

I am unsure. The fields have the dependency to 'currency_digits' at class level, and the method is used for compute the value of the fields.
msg45502 (view) Author: [hidden] (semarie) Date: 2018-12-25.10:15:12
new patch (-4). just a cosmetic change: remove a #TODO comment that shouldn't be here anymore
msg45501 (view) Author: [hidden] (semarie) Date: 2018-12-25.10:10:24
New patch (issue7890-3.patch)
- getter name updated to `get_payment_aggregated`
- label of `amount_succeeded` updated
- all new fields prefixed with `payment_`
- use Sum(..., filter_=...) : I used it only when backend is postgresql: current code in modules uses the same idiom
- uses a method call for the list of states. I think the name could be better ? it is `_get_complete_states` currently
- add conditionals for filling result[], and as here for computes the values (columns of the query are dynamically added depending the fields asked for)
- scenarios extended

New things:
- add a `currency_digits` functional field. as several payment_ fields depends on it, it is better if it is here

Pending question:
- should the state name for `payment_amount_succeeded` be a list and returned by method call ? currently, it is the amount for payment with state `succeeded`. but does a module would be able to extend the semantic to have differents states for "succeeded" ? it could be changed later too.

Tested with tox on py36-{sqlite,postgresql}
msg45447 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-12-21.19:07:44
I do not think the getter name should list all the fields because we will have to change name if we add new one. Maybe get_payment_aggregated.
The label of 'amount_succeeded' should be "Amount Succeeded" or just "Succeeded".
I think it is better to prefix all those field with 'payment_'
Instead of Sum(Case()), it is better to use Sum(..., filter_=...)
Why naming group id 'grid'?
I think SQLite needs also to convert integer result into Boolean.
Still the list of states in the query should be extendable, this means it should come from a method call. And it should be reused in the searcher.
Why not fill directly result like this: result['count'][id_] = count ? Also you can also avoid to fill dict for names that are not requested.
It will be good to extend scenario to test the computation of those field. No need to create more records but just run the methods.
msg45191 (view) Author: [hidden] (semarie) Date: 2018-12-07.14:14:23
> Usually we have getter method with the same name as the function field, so get_payments_info would be named get_amount.

I used this name because the same function was used for getter of count, amount, amount_succeeded, complete. I renamed it.

> I think the getter should use SQL query with grouping because it is not efficient to read all the lines out of the database just aggregate them in Python.

I have redone it using SQL.

> I think it will be more extension friendly to get the states list from a common private function.

Yes, I agree, but should I do it properly ? Is it just adding a classmethod in Payment ? Do you have an example of such case in a module ?

> The new fields should be added at least to the list view to allow searching on them.

ah yes. it was an error on my side: I didn't included the files in the commit.
msg45122 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-12-04.00:57:34
Style remarks: we put help attribute as last argument, we indent closing also the closing brackets.
Usually we have getter method with the same name as the function field, so get_payments_info would be named get_amount.
I think the getter should use SQL query with grouping because it is not efficient to read all the lines out of the database just aggregate them in Python.
I think it will be more extension friendly to get the states list from a common private function.
The new fields should be added at least to the list view to allow searching on them.
msg45099 (view) Author: [hidden] (semarie) Date: 2018-12-02.15:55:45
All new elements are Functions, and computed in one pass on group.payments.

Searching on Complete is done by getting the list of all Payment in incomplete state (not in ['succedded', 'failed']), so the list should remain reasonably short even on big databases. I expect most of Payment to be 'succedded' or 'failed' at some point.
msg45061 (view) Author: [hidden] (semarie) Date: 2018-11-28.10:59:37
Proposal on https://discuss.tryton.org/t/additional-informations-on-payment-group/898
History
Date User Action Args
2020-06-15 11:08:01roundup-botsetmessages: + msg58709
2020-06-15 11:07:56roundup-botsetmessages: + msg58708
2020-06-15 09:02:17roundup-botsetmessages: + msg58703
2020-06-15 09:02:10roundup-botsetstatus: testing -> resolved
nosy: + roundup-bot
messages: + msg58702
2020-05-29 10:40:09reviewbotsetmessages: + msg58359
2020-05-15 20:06:48reviewbotsetmessages: + msg58052
2020-05-15 18:35:26reviewbotsetmessages: + msg58050
2020-05-15 17:32:17reviewbotsetmessages: + msg58046
2020-05-15 13:35:03reviewbotsetnosy: + reviewbot
messages: + msg58042
2020-05-15 13:29:49semariesetfiles: - issue7890-4.patch

Showing 10 items. Show all history (warning: this could be VERY long)