Skip to content

Conversation

@hSaria
Copy link
Contributor

@hSaria hSaria commented Feb 15, 2020

Fixes: #2511

Add a difference section to the object change (i.e. /extras/changelog/{pk}/) with previous and next buttons.

image

While going with a full-blown diff library might provide a nicer-looking diff, like that of GitHub, I've opted for a simpler approach to avoid the additional overhead of another dependency. And it seemed a bit overkill.

@tb-killa
Copy link
Contributor

Doesn't we could use this type of implementation for fixing #3451 (closed but allowed) too?

@hSaria
Copy link
Contributor Author

hSaria commented Feb 16, 2020

I've look into that, though I've got some concerns:

  1. The additional database read call used to read the previous change (this would happen for every object change)
  2. Inconsistency in the webhook data due to settings.CHANGELOG_RETENTION; the previous change might've been deleted if it was too old and thus the webhook data will not have a diff.
  3. Even more inconsistency in the webhook data because not all models log their changes, so some webhooks will have a diff, whilst others won't.

At that point, I'm not even sure it's worth the effort from the perspective of the application that's listening to webhook because it would need to have the measures necessary to handle a webhook without a diff, anyway. And to be honest, a webhook listener should only ever perform idempotent actions, where running the same action multiple times will result in the same state (usually requires extra effort on the action's side), so a diff wouldn't be needed to begin with.

@jeremystretch jeremystretch merged commit 2147354 into netbox-community:develop Feb 19, 2020
@jeremystretch
Copy link
Member

Nice!

@neufeind
Copy link

If I update an object with the same like existing data could this here also make sure to only create a new "version" if at least something changed? Like if I update a server with it's primary ip-address - and if it didn't change to what was already in the database simply don't actually create a "save" for it?

@jeremystretch
Copy link
Member

@neufeind that would be outside the scope of the change proposed in the issue this PR addresses (#2511)

@hSaria hSaria deleted the 2511-change-diff branch February 19, 2020 19:38
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide a diff showing the changes between two logged changes

4 participants