simplyopen-it / django-admin-reports

reports for django-admin
BSD 3-Clause "New" or "Revised" License
65 stars 26 forks source link

Impossible to pass kwargs or args to Report class #10

Open SoulRaven opened 7 years ago

SoulRaven commented 7 years ago

Is impossible to pass any arguments to Report class when is called in this construct:

class BuildDeviceSessionReport(Report):
    fields = ['rx_tx_bytes_total', 'gps_nogps_records', 'created', 'modified', 'session_time', 'session_ip']

    def aggregate(self, **kwargs):
        return DeviceSessionHistory.objects.filter(device_id=3)

class DeviceSessionReport(ReportView):

    license_plate = None

    def __init__(self, *args, **kwargs):
        super(DeviceSessionReport, self).__init__(report_class=BuildDeviceSessionReport, *args, **kwargs)

    def get_context_data(self, **kwargs):
        device_id = kwargs['pk']
        context = super(DeviceSessionReport, self).get_context_data(**kwargs)
        setattr(self, 'device_id', device_id)
        license_plate = DeviceSessionHistory.objects.get_license_plate(device_id=device_id)
        context.update({"title": license_plate})
        return context

using this construct is impossible to pass device_id var on BuildDeviceSessionReport because ReportView is calling report_class without any args.

oberix commented 7 years ago

Sorry I don't get it.

The View passes a valid Form to the Report. The Form must be associated to the Report with form_class attribute, this is clearly (?) told in the README.

Zaharia Constantin writes:

Is impossible to pass any arguments to Report class when is called in this construct:

class BuildDeviceSessionReport(Report):
  fields = ['rx_tx_bytes_total', 'gps_nogps_records', 'created', 'modified', 'session_time', 'session_ip']

  def aggregate(self, **kwargs):
      return DeviceSessionHistory.objects.filter(device_id=3)

class DeviceSessionReport(ReportView):

  license_plate = None

  def __init__(self, *args, **kwargs):
      super(DeviceSessionReport, self).__init__(report_class=BuildDeviceSessionReport, *args, **kwargs)

  def get_context_data(self, **kwargs):
      device_id = kwargs['pk']
      context = super(DeviceSessionReport, self).get_context_data(**kwargs)
      setattr(self, 'device_id', device_id)
      license_plate = DeviceSessionHistory.objects.get_license_plate(device_id=device_id)
      context.update({"title": license_plate})
      return context

using this construct is impossible to pass device_id var on BuildDeviceSessionReport because ReportView is calling report_class without any args.

Marco Pattaro R&D | SimplyOpen SRL http://www.simplyopen.org

SoulRaven commented 7 years ago

I want to pass in my case device_id variabile back to the ReportView, and when ReportView calls report_class=BuildDeviceSessionReport to call BuildDeviceSessionReport with arguments, because now BuildDeviceSessionReport that is extended from Report class is called without arguments.

https://github.com/simplyopen-it/django-admin-reports/blob/master/admin_reports/views.py#L235 https://github.com/simplyopen-it/django-admin-reports/blob/master/admin_reports/views.py#L249

but if Report class has args and kwargs when is called, i have the possibility to pass arguments from url back to the report class

oberix commented 7 years ago

Ok so if I got you right you suggest to pass get() and post() *args, **kwargs to the Report's __init__() so that you can use them in the context, right?

I guess this could be done, but if you extend the ReportView you would loose the urls autodiscover.

SoulRaven commented 7 years ago

no problem, autodiscovery works just fine if the user wants to use out of the box default solution, but if you want to use in extended version, you have to pass some data back, like arguments from url back to the aggregate to make a query based on filter queryset

oberix commented 7 years ago

I think this commit 3d2c6d4f22cba24e344c17b7e7f91551e5454e33 does the trick.

I might be missing something, but I thinks that you could do what you need without extending the view: just overwrite self.title in __init__()

class MyReport(Report):
    def __init__(self, *args, **kwarge):
         super(MyReport, self).__init__(*args, **kwargs)
         self.title = kwargs['title']      # or something similar
SoulRaven commented 7 years ago

is ok, like this, but still is impossible to parse args/kwargs after the report_class variable is populated with Report class extend. Because if you look on my example, from the first post,"device_id = kwargs['pk']" is taken from the url arguments, and needs to be passed back Report extended class, and when https://github.com/simplyopen-it/django-admin-reports/commit/3d2c6d4f22cba24e344c17b7e7f91551e5454e33#diff-eaae985064c6dea11cdcebcb5f26aeb9R249 is called, the the arguments from to be passed, in my opinion, i think is necessary to have a special variable on the view class, that will be passed as args/kwarg when is need,

class BuildDeviceSessionReport(Report):
    fields = ['rx_tx_bytes_total', 'gps_nogps_records', 'created', 'modified', 'session_time', 'session_ip']

    def __init__(self, *args, **kwarg):
        super(BuildDeviceSessionReport, self).__init__(args, kwarg)
        device_id = kwarg.get('device_id')
    def aggregate(self, **kwargs):
        return DeviceSessionHistory.objects.filter(device_id=**device_id**)

class DeviceSessionReport(ReportView):

    license_plate = None

    def __init__(self, *args, **kwargs):
        super(DeviceSessionReport, self).__init__(report_class=BuildDeviceSessionReport, *args, **kwargs)

    def get_context_data(self, **kwargs):
        device_id = kwargs['pk']
        context = super(DeviceSessionReport, self).get_context_data(**kwargs)
        self.report_class_kwargs['device_id'] = device_id
        return context

this is just a prove of concept to understand my idea,

anmolbk commented 3 years ago

Yes, being able to pass URL parameters to the report's view can sometimes be useful, as @soulraven needs. A possible workaround for handling this requirement may be to add the parameter for filtering as a field in the report form (instead of passing it as a URL parameter), and use something like Django Autocomplete Light for filtering dependent fields dynamically.

oberix commented 3 years ago

I made some changes: basically added hooks to ReportView to handle args and kwargs as well as report class. New methods of ReportView are:

They work like FormMixin methods to initialize a form instance (sort of), so if you need some customization you should override these methods.

Since there are this hooks now, I think there's no need to pass request's kwargs to report's __init__ anymore as you can always override get_report_kwargs to do so.

A feedback on this new design would be greatly appreciated. Thanks

anmolbk commented 3 years ago

A feedback on this new design would be greatly appreciated. Thanks

Thanks @oberix , but i am not sure if this solves the need to pass report URL parameters. Let me try to explain my case with an example.

So say we have a company with employees belonging to departments. Now in the changelist view for departments, I add a column (using a callable in department's admin class) with admin report link to open a report listing the department's employee info. Currently the report link is same for all departments. In the report, I have a ModelChoiceField for the employees. As of now, I list all the company employees in this field, as I don't know for which department the report link was clicked. So if I pass the department ID as a parameter to the report URL, and am able to fetch the passed URL parameter in the report form's __init__, I will be able to filter the employee field's queryset to only those employees belonging to the passed department ID.

So basically, i need to communicate with the report via the report link. I think with your new design, this is only possible by creating custom URL pattern, which will break the report autodiscovery as you mentioned earlier. Hope my understanding is correct.

oberix commented 3 years ago

@anmolbk you are right, the autodicover is used to automagically make your application aware of a module called reports.py (it's what the django admin uses to find admin.py). The idea behind this was to mimic the admin behavior, and just like the admin, if you want to pass extra arguments to an admin class you'll have to override all the pipeline from url patterns down through a custom view. The task is quite simple actually:

I'm not very much keen to pass view's kwargs to reports by default because they are not specifically meant for that purpose, some view may have multiple arguments and not all of them to be passed to the report.

If you want to avoid the drag of all the override and extend shown above, you may use GET params that will be used as form's values "naturally", just make the corresponding form field hidden if it's not supposed to be something a user should mess with.

anmolbk commented 3 years ago

So as per @oberix 's previous comment, to summarize, there are two solutions for this requirement as I understand. Choose the appropriate one as per your case:

  1. Custom URL & view: Create your own custom report URL pattern taking the URL parameter(s) as you require. Create a custom report view extending ReportView to handle the URL and override get_report_kwargs to pass on the parameters to the report's form class. Note that this is in line with admin behavior. See @oberix 's comment for details.
  2. Add report form fields: Instead of passing the report input as URL parameter(s), add corresponding field(s) for the parameter(s) in your report form to let user to select the values in the report form itself. If you need to change any other data based on user selection of the added field, you can use some kind of AJAX autocomplete (eg. Django Autocomplete Light package). So in case of my company-department-employee example in my previous comment, I would add a 'department' ModelChoiceField in the report form (instead of passing it as a report URL parameter). The employees field would then be filtered using AJAX autocomplete on department selection change.

I think with this, the issue may be closed. Thanks @oberix!