Re: Adding aggregates to ModelAdmin.list_display

2016-09-30 Thread dor 
Can we agree on the suggested design?
Or does anyone think it needs to be incorporated into *list_display*?

On Wednesday, September 21, 2016 at 7:43:06 PM UTC+3, dor wrote:
>
> I recently suggested adding a *list_aggregates* member to ModelAdmin, 
> along with a patch to match:
> https://code.djangoproject.com/ticket/27229
>
> A core developer counter-suggested changing the design of list_display to 
> achieve the same;
>
> What do you think?
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/f84765d6-32bc-477c-b45f-8a7683cadb28%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Adding aggregates to ModelAdmin.list_display

2016-09-24 Thread dor 
I see much value in aggregating on a single page (rather than the entire 
QS).
Check out this 
example: 
https://cloud.githubusercontent.com/assets/1499307/18555738/964622e0-7b71-11e6-9a53-3a525ba25b4b.png

Also, my current implementation (in the pull request) does support a 
user-friendly name for the field, like so:
class EmployeeAdmin(admin.ModelAdmin):
list_display = ('name', 'age', 'formatted_monthly_salary')
list_aggregates = (('age', Avg), ('formatted_monthly_salary', Sum))

def formatted_monthly_salary(self, employee_or_salary):
salary = getattr(employee_or_salary, 'monthly_salary', 
employee_or_salary)
return '${0:,.2f}'.format(salary)

formatted_monthly_salary.admin_aggregate_field = 'monthly_salary'



On Thursday, September 22, 2016 at 8:52:20 PM UTC+3, Andrew Mosson wrote:
>
> We have an implementation of both annotations in *list_display* and 
> adding an aggregates for the entire list (which we call *list_summary* 
> and what you are calling here *list_aggregates*) and there are a bunch of 
> subtleties in the interaction due to Admin's built in support for 
> pagination and filtering
>
> To demonstrate, let's use a simplified use case
> assume models such as Author -< Book >- Genre (Book has FKs to both 
> Author and Genre)
>
> and and admin such as (as suggested in Stack Overflow post that Tim 
> referenced (​already possible to some extent  
> 
> )
>
> class AuthorAdmin(admin.ModelAdmin):
> list_display = ('author_name', 'book_count', 'book_sum_price')
> list_filters = ('book__genre__name', )
> list_summary = (('Total Books', Sum('book_count'), ),
> ('Total Book Price', Sum('book_sum_price'), ))
> 
> def get_queryset(self, request):
> return super(BookAdmin, self).get_queryset(request).annotate(
> books_count=Count('books__name'),
> books_sum_price=Sum('books__price'))
>
> With regards to *list_summary* (or *list_aggregates* in the PR) our 
> implementation summarizes the entire QuerySet not just a single page (my 
> quick read of the patch seems to indicate that list_aggregates only 
> aggregates a single page of the qs).  From my perspective summarizing a 
> single page doesn't provide as much value summarizing the entire QS.  If 
> one agrees  with that then feature will would have to support a user 
> friendly name for the field (implemented here as a tuple - as suggested by 
> Jim).  Additionally, if the feature summarizes the entire qs then the 
> output should likely go above the table rather than as summary below (if it 
> were below and the results were paginated, it would likely confuse the 
> users)
>
> With regards to extending list_display to support annotations there are 
> subtle interactions between admin, annotated query sets and filters.
>
> The qs that Django executes when the user has a filter looks like
>
> Author.objects.annotate(...).filter(...)
>
> In the code shown above this will produce the correct result set, but 
> because annotate and filter aren't commutative 
> ,
>  
> the generated SQL ends up joining the Books table twice.  Additionally, 
> there are probably some cases where complex filters will give unexpected 
> results.  
>
> Give that the user really wants
>
> Author.objects.filter(...).annotate(...)
>
> we ended up adding a modelAdmin.get_annotations(...) method and 
> subclassing ChangeList to implement this feature.  
>
> One additional note is that annotations require implementing a method on 
> the modelAdmin class for each annotations which seems very boilerplate-ish. 
> We have extended *list_display* to automatically handle the boilerplate 
> as well.  
>
> Since we use this extensively these features in our applications, we would 
> be excited to see them implemented as part of admin.  We'd be happy to 
> contribute here and if this seems like its worth pursuing I'll ask our team 
> to look into refactoring the work we've done so that it could live in core 
> rather than as sub-classes.
>
> On Wednesday, September 21, 2016 at 6:17:27 PM UTC-7, Josh Smeaton wrote:
>>
>> I think I'm OK with `list_aggregates` because it implies a terminal 
>> queryset method which really restricts the members used to create that 
>> aggregation (the GROUP BY). Adding aggregates to existing list_display 
>> would require something *else* to refine the group by using `values()`.
>>
>> If list_aggregates is a useful feature, then this sounds like an 
>> appropriate way to implement that. Regular annotations could be added and 
>> processed within list_display, provided list_display was modified to accept 
>> expressions (either aggregates or regular annotations) in tuple form for 
>> alias support.
>>
>> list_aggregates -> queryset.aggregate()
>> list_display -> queryset.anno

Re: Adding aggregates to ModelAdmin.list_display

2016-09-22 Thread Andrew Mosson 
We have an implementation of both annotations in *list_display* and adding 
an aggregates for the entire list (which we call *list_summary* and what 
you are calling here *list_aggregates*) and there are a bunch of subtleties 
in the interaction due to Admin's built in support for pagination and 
filtering

To demonstrate, let's use a simplified use case
assume models such as Author -< Book >- Genre (Book has FKs to both Author 
and Genre)

and and admin such as (as suggested in Stack Overflow post that Tim 
referenced (​already possible to some extent  

)

class AuthorAdmin(admin.ModelAdmin):
list_display = ('author_name', 'book_count', 'book_sum_price')
list_filters = ('book__genre__name', )
list_summary = (('Total Books', Sum('book_count'), ),
('Total Book Price', Sum('book_sum_price'), ))

def get_queryset(self, request):
return super(BookAdmin, self).get_queryset(request).annotate(
books_count=Count('books__name'),
books_sum_price=Sum('books__price'))

With regards to *list_summary* (or *list_aggregates* in the PR) our 
implementation summarizes the entire QuerySet not just a single page (my 
quick read of the patch seems to indicate that list_aggregates only 
aggregates a single page of the qs).  From my perspective summarizing a 
single page doesn't provide as much value summarizing the entire QS.  If 
one agrees  with that then feature will would have to support a user 
friendly name for the field (implemented here as a tuple - as suggested by 
Jim).  Additionally, if the feature summarizes the entire qs then the 
output should likely go above the table rather than as summary below (if it 
were below and the results were paginated, it would likely confuse the 
users)

With regards to extending list_display to support annotations there are 
subtle interactions between admin, annotated query sets and filters.

The qs that Django executes when the user has a filter looks like

Author.objects.annotate(...).filter(...)

In the code shown above this will produce the correct result set, but 
because annotate and filter aren't commutative 
,
 
the generated SQL ends up joining the Books table twice.  Additionally, 
there are probably some cases where complex filters will give unexpected 
results.  

Give that the user really wants

Author.objects.filter(...).annotate(...)

we ended up adding a modelAdmin.get_annotations(...) method and subclassing 
ChangeList to implement this feature.  

One additional note is that annotations require implementing a method on 
the modelAdmin class for each annotations which seems very boilerplate-ish. 
We have extended *list_display* to automatically handle the boilerplate as 
well.  

Since we use this extensively these features in our applications, we would 
be excited to see them implemented as part of admin.  We'd be happy to 
contribute here and if this seems like its worth pursuing I'll ask our team 
to look into refactoring the work we've done so that it could live in core 
rather than as sub-classes.

On Wednesday, September 21, 2016 at 6:17:27 PM UTC-7, Josh Smeaton wrote:
>
> I think I'm OK with `list_aggregates` because it implies a terminal 
> queryset method which really restricts the members used to create that 
> aggregation (the GROUP BY). Adding aggregates to existing list_display 
> would require something *else* to refine the group by using `values()`.
>
> If list_aggregates is a useful feature, then this sounds like an 
> appropriate way to implement that. Regular annotations could be added and 
> processed within list_display, provided list_display was modified to accept 
> expressions (either aggregates or regular annotations) in tuple form for 
> alias support.
>
> list_aggregates -> queryset.aggregate()
> list_display -> queryset.annotate(annotations).values()
>
> Does that make sense?
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/845eeb78-cebe-44cf-9ba7-a36c69522149%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Adding aggregates to ModelAdmin.list_display

2016-09-21 Thread Josh Smeaton 
I think I'm OK with `list_aggregates` because it implies a terminal 
queryset method which really restricts the members used to create that 
aggregation (the GROUP BY). Adding aggregates to existing list_display 
would require something *else* to refine the group by using `values()`.

If list_aggregates is a useful feature, then this sounds like an 
appropriate way to implement that. Regular annotations could be added and 
processed within list_display, provided list_display was modified to accept 
expressions (either aggregates or regular annotations) in tuple form for 
alias support.

list_aggregates -> queryset.aggregate()
list_display -> queryset.annotate(annotations).values()

Does that make sense?

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/95509037-34d2-4747-abab-64e319db292c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Adding aggregates to ModelAdmin.list_display

2016-09-21 Thread Tim Graham 
How do you envision it interacting with ticket #14336 (annotations in 
list_display)? Would we also need list_annotate? I'm a little sad every 
time we add another customization point to the admin as I fear no end to 
the complexity, but maybe I should just get over it.

https://code.djangoproject.com/ticket/14336

On Wednesday, September 21, 2016 at 12:43:06 PM UTC-4, dor wrote:
>
> I recently suggested adding a *list_aggregates* member to ModelAdmin, 
> along with a patch to match:
> https://code.djangoproject.com/ticket/27229
>
> A core developer counter-suggested changing the design of list_display to 
> achieve the same;
>
> What do you think?
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/9d6f1cab-b513-4c0b-95bc-c53c6b0dff0b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Adding aggregates to ModelAdmin.list_display

2016-09-21 Thread dor 
I recently suggested adding a *list_aggregates* member to ModelAdmin, along 
with a patch to match:
https://code.djangoproject.com/ticket/27229

A core developer counter-suggested changing the design of list_display to 
achieve the same;

What do you think?

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/30e97144-e75e-4c07-a601-6b116a1f681c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.