-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Does not work with django-taggit #10
Comments
Thanks for reporting this! Hm. I have used django-tagging in the past. But this is also as django-taggit not really up to date :( Maybe django-tagging-ng is the currently the only maintained tag solution? See: https://www.djangopackages.com/grids/g/tagging/ So, i question me, how django-tagging-ng works with reversion-compare... |
I've patched my local admin.py file to check for |
Seems that's a general problem, see: #12 |
This also happened for my code. I solved it by changing it to:
|
@velmont: IMHO this should do the same:
In the next few weeks i have no time to test this and create a unittest... |
Absolutely, it's a much better pattern and I have no idea how I could've missed it. I did fix it two places though, and the second virtualenv I fixed it in I indeed did the get version :-) I am not sure if it is the right fix to set the value to the empty string if nothing is found, but it does sound correct. What do you think? Is it the real fix? I have no time right now, but if it is the real one, someone else might do a test and fix, -- or I might be able to do one during the week. |
Any news on this issues? Did this happen in the current release or can this been closed?!? |
I'm using my fork as dependency in the project where I'm doing this, haven't checked the new version. Don't have the data it crashes on handy, so it might take me some time to figure out the answer to that question. But based on reading the code that's now in admin.py, it seems like it would work (you've seemed to have implemented the fix, but with "Field doesn't exist!" as value instead of ""). |
Dear All, Using:
I got this error in compare.py line 155:
Ideas? D |
A monkey patch for this issue: from reversion_compare.mixins import CompareMixin
from django.db.models import Manager
_old_compare = CompareMixin.compare
def compare(self, obj, version1, version2):
def replace_taggit_field(version_ins):
for fieldname in version_ins.field_dict:
if isinstance(version_ins.field_dict[fieldname], Manager):
version_ins.field_dict[fieldname] = []
replace_taggit_field(version1)
replace_taggit_field(version2)
return _old_compare(self, obj, version1, version2)
CompareMixin.compare = compare Put those code at the start lines of your |
@sadnoodles can you contribute a pull request with a real fix? |
@jedie I'm glad to, but I'm not sure this is the solution. This patch works for me. Basic its just a custom field problem. And in this case, in taggit, it is a M2M Manager, some may be more strange. I'm thinking a presetting function that can be customized would be a good idea, but I not sure where to put it for now. |
Django-reversion-compare seems to have a good deal of problems with django-taggit; perhaps it's due to django-taggit not being up-to-date enough with Django 1.4.5, but I tried both yedpodtrzitko's and shakirjames's and both exhibit the same issue.
Django 1.4.5 with a model as following:
django-reversion-compare throws a
KeyError
at reversion_compare/admin.py:48:Changing this line to
version.field_dict.get(field_name, None)
makes it go down a rabbit-hole of issues with TaggableManager not providingget_internal_type
which d-r-c makes use of frequently, eg:Is the right solution to check in all places where
get_internal_type
is used to ensure the fieldhasattr(field, 'get_internal_type')
?The text was updated successfully, but these errors were encountered: