-
Notifications
You must be signed in to change notification settings - Fork 17
Polish changes page #421
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
base: main
Are you sure you want to change the base?
Polish changes page #421
Conversation
brainexe
commented
Dec 18, 2025
- add filter for "attribute"
- show all attribute changes with a toggle
- fixed pagination (all filters were dropped on click)
- add link to app/user for fast filtering
- filter by attribute - show attribute changes with a toggle - fixed pagination - link on app/user for fast filtering
kofrezo
left a comment
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 originally did not include an attribute filter because the queries are rather expensive having a strong impact on the database cache slowing down everyday queries (Servershell + API). But we can give it a try and hope the feature does not get used all the time.
The rest looks okay to me. Nice improvements - thank you. You could check my comments and see if you could replace some of the custom methods with Django builtins.
| newer = _validate_commit(changed, changed_objects) | ||
| if newer: | ||
| raise CommitNewerData('Newer data available', newer) | ||
| raise CommitNewerData(f'Newer data available for attribute {newer}', newer) |
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 is absolutely unrelated to polishing of the changes page no ?
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.
yea, a side change I applied during testing
| return ', '.join(str(v) for v in value) | ||
| if isinstance(value, set): | ||
| return ', '.join(str(v) for v in sorted(value)) | ||
| return str(value) |
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 method is extra. It can be done using the builtin default template tag (here) and __str__ method.
| prefix = f'<strong>{escape(attr_name)}:</strong>' | ||
| action = attr_change.get('action') | ||
| old_val = escape(_format_value(attr_change.get('old'))) | ||
| new_val = escape(_format_value(attr_change.get('new'))) |
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.
If you want to escape a whole text you can also use the escape template filter (here).
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.
but then it would also escape the wanted html tags like etc
- review changes - Inspect page: add link to history of current object
