search

burke-software / django-mass-edit

Make bulk changes in the Django admin interface
152 stars 67 forks source link

Use session to store long objects_ids string #62

Closed variant77 closed 7 years ago

variant77 commented 8 years ago

When many objects are selected for mass edit the URL string can get very long, especially if the model's primary key has many digits or id a uuid. At some point the URL becomes too long and this will result in a server error.

My solution checks if the object_ids string is over 500 characters (We might want to have that number configurable) and if it is so stores the comma separated string in the request session instead of passing it along.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-1.3%) to 89.785% when pulling 9c90eda9c4b84533cf6c4b393e0a1292b7faaa6f on variant77:master into 38b2b28c997a4d4c1998f51ff4989c00b60edb8e on burke-software:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-1.3%) to 89.785% when pulling 42466e1268b3d76bfd1189c0d17ffd429d7ada12 on variant77:master into 38b2b28c997a4d4c1998f51ff4989c00b60edb8e on burke-software:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-6.7%) to 84.375% when pulling 4bad95db51464051dc5fe1c347d11354492c0ad0 on variant77:master into 38b2b28c997a4d4c1998f51ff4989c00b60edb8e on burke-software:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-4.7%) to 86.458% when pulling 4bad95db51464051dc5fe1c347d11354492c0ad0 on variant77:master into 38b2b28c997a4d4c1998f51ff4989c00b60edb8e on burke-software:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-1.6%) to 86.458% when pulling eb78d8dc26b8796d8e460658e6a30ad436392041 on variant77:master into 32e6dbe1255e6c2ed04bfbfc54d20467003ebdec on burke-software:master.

variant77 commented 8 years ago

I am happy to see the interest in my Pull Request and the discussion here (https://github.com/burke-software/django-mass-edit/pull/64). I would like to address the points raised:

  1. Although opening multiple tabs in not a common scenario in submit based admin actions and the integrity of the session data important only for the time of the redirect I updated my Pull request to use a hash of the object ids string as per @PetrDlouhy 's suggestion. (https://github.com/burke-software/django-mass-edit/pull/62/commits/9c90eda9c4b84533cf6c4b393e0a1292b7faaa6f)
  2. I would like to argue in favor of my approach to use the session storage only if the URL is longer than a certain length: In small data sets I can see the use of bookmarking or sharing the URL to mass edit a collection of items. This becomes irrelevant in larger growing tables. Furthermore, it keeps backward compatibility for anyone who have already bookmarked such a set.
  3. Sessions are enabled by default when using django-admin and I am almost sure they are required for use with django auth.
PetrDlouhy commented 8 years ago

OH, I didn't realize, that there is already similar PR to my own. Good work @variant77, I think that your's approach clearly goes further and is more faultproof. I think, that using hash is necessary and using sessions is only design choice which I think is not bad.

Although, my PR contains tests of the object_ids handling through sessions, so I think you could incorporate into your PR.

PetrDlouhy commented 8 years ago

And one other think - the dist/django-mass-edit-2.6.tar.gz and dist/django_mass_edit-2.6-py2.7.egg files in your PR are there probably by mistake.

PetrDlouhy commented 8 years ago

I have got following error with your code on Python 3, Django 1.9:

Unicode-objects must be encoded before hashing

on line 69 in django-mass-edit/massadmin/massadmin.py

hash_id = "session-%s" % hashlib.md5(object_ids).hexdigest()
coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.9%) to 87.179% when pulling 71c02b2e4a181c2dfbf7e18ab4c9baacefb00f4c on variant77:master into 32e6dbe1255e6c2ed04bfbfc54d20467003ebdec on burke-software:master.

variant77 commented 8 years ago

Yes. they were by mistake. My first time contributing on github and didn't realize subsequent commits are also added to the PR. I rebased to remove them.

Also updated the tests to support sessions and added a test to check with many objects. Will fix the Python 3 compatability soon.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+1.2%) to 89.231% when pulling f4d77ec1949a0041e9ba12fe4da052f8767c6805 on variant77:master into 32e6dbe1255e6c2ed04bfbfc54d20467003ebdec on burke-software:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+1.2%) to 89.231% when pulling f4d77ec1949a0041e9ba12fe4da052f8767c6805 on variant77:master into 32e6dbe1255e6c2ed04bfbfc54d20467003ebdec on burke-software:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+1.2%) to 89.231% when pulling f4d77ec1949a0041e9ba12fe4da052f8767c6805 on variant77:master into 32e6dbe1255e6c2ed04bfbfc54d20467003ebdec on burke-software:master.

PetrDlouhy commented 7 years ago

@variant77, @bufke Is there anything preventing this PR from pulling?

variant77 commented 7 years ago

As far as I'm concerned it's ready. I'm using my fork in production and everyone is happy with finally being able to work with large sets.

PetrDlouhy commented 7 years ago

@variant77: I have made an standalone application from this code, so it could be used also for other purposes: https://github.com/PetrDlouhy/django-admin-large-initial-data

@bufke: How about merging this?

PetrDlouhy commented 7 years ago

@bufke Any news about this?

bufke commented 7 years ago

As I read it, this change solves the multiple tabs issue discussed on #64 right? Has anyone tested this out? This pull request should be rebased, it appears to contain some unrelated commits that are already in master.

It remains a disadvantage that this change forces people to use sessions right? Is there any potential problem with this? Can django admin even be used without sessions? I could use token or basic auth with django admin I think, though I don't know if people really do this or not. This project has 65 stars so probably not a huge active user base. Still we'd want to note the potential breaking change.

If anyone else wants to join this project as a maintainer let me know too. Once we can review these considerations I think I can merge this.

variant77 commented 7 years ago

Yes. This resolves the issue of multiple tabs as the session field is a hash of the object_ids.

According to the docs (Overview section 2) django.contrib.sessions is a dependency for django admin so we're covered there.

We can make this behavior optional or configurable but I don't think there is any need for it. old-style urls still work if you have them bookmarked so there is no real breaking change.

bufke commented 7 years ago

Released as 3.0. Please try it out.