magopian / django-data-exports

Model data exports for Django
http://django-data-exports.rtfd.org
BSD 3-Clause "New" or "Revised" License
39 stars 12 forks source link

Small update I needed (perf + queryset filtering) #7

Closed Christophe31 closed 10 years ago

Christophe31 commented 10 years ago

I needed this feature and my complex data structure made me also need the small performance fix.

magopian commented 10 years ago

Hello Christophe,

thanks for this work.

I'm not 100% sure this feature will benefit a lot of people. It seems like a pretty specific need, and could easily be achieved by subclassing the ExportView, what do you think?

Regarding the performance fix, I'm pretty sure add_fields is only called once on the created formset (at least in Django < 1.6), so putting the code in __init__ or in add_fields should be the same, right? Also, it's only working with Django1.6 according to Travis, not sure if it can be fixed on other versions?

Christophe31 commented 10 years ago

I got gateway timeout before the performance fix, now I have my page. My list of related fields have over 1500 entries so the loading so the impact was substantial. Even if i didn't do any tracing or measured loading.

What the 'index' argument is if the method is only called once?

For the queryset, I was thinking about a way to filter for the user or by permission or anything for common security purposes.

You think inheriting views is easier? I may be wrong but I never use class views and I think most django developers still make simple view functions and make common features with decorators.

Christophe31 commented 10 years ago
 def get_context_data(self, **kwargs):
     context = super(ExportView, self).get_context_data(**kwargs)
     model = self.object.model.model_class()
     context['data'] = model.objects.all()
     return context

Your view don't even give direct access to the queryset. The user must inherit, get context and edit data entry's queryset. For the filter will certainly be model based, I don't want to create an url entry and a view per exportable model. And if I filter on view, I must filter following model data. So I have to ensure the model in my view to allow the request? If I handle this on model rather than on view, I'm sure no one will export my model with a view with low/no queryset filter.

Christophe31 commented 10 years ago

I'll take a look on feature changes in FormSets for django 1.6 but...

The instance method is called, before/without __init__ call in older django version? o_O

edit: Ok, add_fields was part of the __init__ but isn't since django 1.6

So we can set a memoize feature.

Christophe31 commented 10 years ago

I can argue more via mumble/skype (in french or english) if you want, I think this pull request make your project better. I were you I would ask me to add documentation before thinking about accepting it. I still expect feedback from you after my three last comments

magopian commented 10 years ago

I'm really really sorry, I just didn't get any time to look into this any further, but I promise I will... I just don't know when :(

Thanks a lot for your patience and contribution, I'll get back to you as soon as I possibly can!

Christophe31 commented 10 years ago

As pip allow me to install from my git repo, no need to hurry ^^.

Christophe31 commented 10 years ago

still unavailable?

Christophe31 commented 10 years ago

still unavailable?

magopian commented 10 years ago

ah, sorry about that, i'm still very very busy, I definitely need to find time for this

2014-05-28 9:35 GMT+02:00 Narbonne notifications@github.com:

still unavailable?

— Reply to this email directly or view it on GitHubhttps://github.com/magopian/django-data-exports/pull/7#issuecomment-44374277 .

Christophe31 commented 10 years ago

what? I only updated the README.rst file and CI fail?

Christophe31 commented 10 years ago

I suppose flake8 does'nt exclude migrations anymore.

Christophe31 commented 10 years ago

Still no news?

Christophe31 commented 10 years ago

Still unavailable? This issue started half a year from now.

Christophe31 commented 10 years ago

As the travis fix was not pulled yet, I continued on same pull request.

Christophe31 commented 10 years ago

last commit fix deprecation warning in django 1.7.

magopian commented 10 years ago

Thank you very much @Christophe31 for your work... and your patience ;)

Christophe31 commented 10 years ago

Yay! accepted ;-) I'll be able to use pypi again, thank you too.

magopian commented 10 years ago

Will release on pypi in a few minutes ;)

magopian commented 10 years ago

Released: https://pypi.python.org/pypi/django-data-exports/0.7