-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fixes #14329: Improve diffs for custom fields #14332
base: develop
Are you sure you want to change the base?
Conversation
This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken. |
@arthanson @jeffgdotorg and I talked about this today. The consensus was that we need to address the inconsistency in how the summary difference and the pre-/post-change data are presented. Looking at the screenshot below, note that while the "difference" panel is accurate, the changes highlighted in the pre- & post-change panels still show the entire IMO the pre-/post-change panel highlights should exactly match the summary above, or we should remove the highlighting in that section. |
I've implemented the requested changes: (Only works for Sadly I wasn't able to find the reason that |
I've got it working in a somewhat unelegant manner. I take the list of
And within django templates I parse this and transform this into JSON like that:
sadly this won't work with MultiObject Custom Fields, as it would end up like this:
For that I've created a custom template tag. It transforms a string by:
I don't claim this is the best solution, but it's honestly the only one I could come up with. I'm open for alternative ideas how to make this work |
ff5e9e6
to
aaf1124
Compare
Hello @jeremystretch, I would love to get this merged in near future. Can you either review this (Especially how I solved those diffs in the Pre/Post Change Data!) please or give me a list of open points that are missing? |
8e51bcc
to
d765d95
Compare
netbox/extras/views.py
Outdated
cfr_list = [] | ||
if custom_fields_added: | ||
for cf, cf_value in prechange_data['custom_fields'].items(): | ||
cfr_list.append((cf, cf_value, cf in custom_fields_added)) | ||
cfa_list = [] | ||
if custom_fields_removed: | ||
for cf, cf_value in instance.postchange_data['custom_fields'].items(): | ||
cfa_list.append((cf, cf_value, cf in custom_fields_removed)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this sort of approach isn't going to be maintainable long-term. The diffing implementation needs to operate generically; we can't give custom fields specifically any special treatment. (Consider what changes would be needed to support some similarly structured field in a future release.) Ultimately, we want a solution that can follow nested data structures and roll back up their diffs, without any context about the data contained within.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have one last idea how to implement this in a sane manner:
- Dump "Object before" and "Object after into a formatted JSON-string => One Attribute = One line (Except for arrays/nested objects).
- Diff line by line (With a bit of smartness for arrays/objects) into two lists
removed
andadded
that are just lists of(line_as_string, has_changed)
- Convert the lists to HTML within the templates
Do you think this could be a more maintainable approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this script: https://gist.github.com/JCWasmx86/95f449abf2a8958b22d0e48279b07ba7
Do you think that goes into the right direction? It's a bit less efficient than it could be, but I think it gives good results, e.g.
It works some-what generic on strings, but a sorted JSON-like thing (Like in the changelog pre/post data), it should work great
return substr | ||
|
||
|
||
def make_diff(old: str, new: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will have to be cleaned up a bit, if you think this approach is better. If you don't think this approach has a future, this PR can be closed, as I'm out of ideas.
We basically just convert two strings (Before/After) into two lists of tuples (str, bool).
And it needs a few more tests, but I only write them if this approach has a future
Fixes: #14329
This improves the diff by comparing the changes dicts recursively. It changes the Diff and the Pre/Post-Change Data
That's one issue I found, but it seems unrelated, as even without my patch, the type of the floating-point field seems to change: