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.
Sebastien Marieadded 1 deleted label and removed 1 deleted label
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.
Cédric Krieradded 1 deleted label and removed 1 deleted label
> 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.
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.
New patch (#7890 (closed)-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.
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.
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.