netzkolchose / django-computedfields

Provides autogenerated autoupdated database fields for model methods.
MIT License
94 stars 14 forks source link

fix update_fields in handler #88

Closed jerch closed 2 years ago

jerch commented 2 years ago

Fixes #87.

@stephrdev I was not able to repro #87. Can you give more insights how this was triggered in your code? For all I tried, kwargs['update_fields'] was either None or a frozenset. Which Django version are you using?

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1688694270


Totals Coverage Status
Change from base Build 1639501652: 0.0%
Covered Lines: 1171
Relevant Lines: 1198

💛 - Coveralls
stephrdev commented 2 years ago

I had this issue together with another library (admin-sortable2), the signal is triggered here: https://github.com/jrief/django-admin-sortable2/blob/master/adminsortable2/admin.py#L341

Your fix looks great and should solve the issue!

jerch commented 2 years ago

@stephrdev Thx for checking, guess I'll do a patch release for this.

The usage in django-admin-sortable2 looks wrong to me following the official django docs, which states in https://docs.djangoproject.com/en/4.0/ref/signals/#pre-save:

update_fields The set of fields to update as passed to Model.save(), or None if update_fields wasn’t passed to save().

Note the word set. I also checked the django repo sources and the internals always emit a set like type if it contains anything. Maybe this is worth a bug report in django-admin-sortable2.

stephrdev commented 2 years ago

Thanks for merging and releasing!