Tryton - Issues

 

Issue7890

Title Additional informations on Payment Group
Priority feature Status testing
Superseder Nosy List ced, semarie
Type feature request Components account_payment
Assigned To semarie Keywords
Reviews

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

Files
File name Uploaded Type Edit Remove
issue7890-4.patch semarie, 2018-12-25.10:15:12 text/plain
Messages
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
2019-03-16 15:26:40cedsetkeyword: - patch
2018-12-30 23:41:58cedsetmessages: + msg45638
2018-12-25 10:20:50semariesetmessages: + msg45503
2018-12-25 10:15:18semariesetfiles: - issue7890-3.patch
2018-12-25 10:15:12semariesetfiles: + issue7890-4.patch
messages: + msg45502
2018-12-25 10:10:33semariesetfiles: - account_payment-2.patch
2018-12-25 10:10:25semariesetfiles: + issue7890-3.patch
messages: + msg45501
2018-12-21 19:07:45cedsetmessages: + msg45447
2018-12-07 14:15:10semariesetfiles: - account_payment-1.patch
2018-12-07 14:14:24semariesetfiles: + account_payment-2.patch
messages: + msg45191
2018-12-04 00:57:35cedsetstatus: chatting -> testing
nosy: + ced
messages: + msg45122
2018-12-02 15:55:46semariesetfiles: + account_payment-1.patch
status: unread -> chatting
messages: + msg45099
keyword: + patch
2018-11-28 10:59:37semariecreate