Fix for unintuitive merge logic when no fields are selected. #1
Conversation
src/adminactions/api.py
Outdated
| Finally ``other`` will be deleted and master will be preserved | ||
|
|
||
| @param master: Model instance | ||
| @param other: Model instance | ||
| @param fields: list of fieldnames to merge | ||
| @param commit: bool |
There was a problem hiding this comment.
Feels like the order of parameter types should match the order in the parameter list of the function?
| @param m2m: list of m2m fields to merge. If empty will be removed | ||
| @param related: list of related fieldnames to merge. If empty will be removed | ||
| @return: | ||
| """ | ||
|
|
||
| fields = fields or [f.name for f in master._meta.fields] | ||
| # Ensure we have a valid iterable. | ||
| fields = fields if fields else [] |
There was a problem hiding this comment.
Just curious, why stray from fields or []?
There was a problem hiding this comment.
Got some feedback from Andy on a previous PR about the "or" syntax; he mentioned preferring if/else.
There was a problem hiding this comment.
Looks like Django source code uses:
if fields is None:
fields = []
I would drop # Ensure that we have a valid iterable. since there isn't an actual check whether we can iterate over fields.
| @@ -73,7 +73,7 @@ class Media: | |||
| js = [ | |||
| 'admin/js/vendor/jquery/jquery.js', | |||
| 'admin/js/jquery.init.js', | |||
| 'adminactions/js/merge.min.js', | |||
There was a problem hiding this comment.
What's the reason to no longer use the minified version?
There was a problem hiding this comment.
Tested this with the minified vs the plain, and the minified had the original bug in it while the plain worked as expected.
Good call though, I'll put in a bit of effort to see if I can get the minified version working; no use having a phantom bug waiting for us down the line.
There was a problem hiding this comment.
Hmm, yeah just pushed tried to minify it myself (UglifyJS 3) and the minified version doesn't work :(
Might be worth it to disable some minification options and try a few more times.
There was a problem hiding this comment.
Would you min removing the minified version then? Somebody (most likely me) will come along shortly and try to switch to it.
|
@tvogels01 verified that minification breaks |
By default, the merge feature assumed the user wanted to merge all
fields when no fields were provided. This was inconsistent with the
merge preview display, and surprising to the user.