Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#6117 closed (fixed)

newforms-admin change history needs to be implemented

Reported by: Karen Tracey <kmtracey@…> Owned by: Brian Rosner
Component: contrib.admin Version: newforms-admin
Severity: Keywords: change history nfa-blocker
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

This code in newforms-admin/django/contrib/admin/options.py save_change needs to be replaced with a real implementation:

        # Construct the change message. TODO: Temporarily commented-out,
        # as manipulator object doesn't exist anymore, and we don't yet
        # have a way to get fields_added, fields_changed, fields_deleted.
        change_message = []
        #if manipulator.fields_added:
            #change_message.append(_('Added %s.') % get_text_list(manipulator.fields_added, _('and')))
        #if manipulator.fields_changed:
            #change_message.append(_('Changed %s.') % get_text_list(manipulator.fields_changed, _('and')))
        #if manipulator.fields_deleted:
            #change_message.append(_('Deleted %s.') % get_text_list(manipulator.fields_deleted, _('and')))
        #change_message = ' '.join(change_message)
        if not change_message:
            change_message = _('No fields changed.')
        LogEntry.objects.log_action(request.user.id, ContentType.objects.get_for_model(model).id, pk_value, force_unicode(new_object), CHANGE, change_message)

Until then all the history for changes made using newforms-admin displays "No fields changed."

Attachments (7)

admin_log.diff (2.6 KB) - added by Alex Gaynor 9 years ago.
This should work, brosner has said this should probably be in modelforms, I'm not familliar enough to work on those, however someone can use this work.
admin_log.2.diff (2.6 KB) - added by Alex Gaynor 9 years ago.
Made it a bit more efficient
admin_log.3.diff (5.5 KB) - added by Karen Tracey <kmtracey@…> 9 years ago.
admin_log.4.diff (5.6 KB) - added by Karen Tracey <kmtracey@…> 9 years ago.
Fix error when adding 1st inline object, include quotes around object values like old code did (noticed during investigation of #6853)
admin_changed.diff (8.5 KB) - added by Alex Gaynor 9 years ago.
Added some tests, there may be some test failures, I can't tell right now
admin_changed.2.diff (8.0 KB) - added by Alex Gaynor 9 years ago.
Added some tests, there may be some test failures, I can't tell right now
6117.diff (9.0 KB) - added by Alex Gaynor 9 years ago.
This fixes the test failures from the previous patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by Karen Tracey <kmtracey@…>

comment:2 Changed 9 years ago by Brian Rosner

Keywords: nfa-blocker added
Triage Stage: UnreviewedAccepted

This should be completed before merging since it will completely break the compatibility with the trunk admin.

comment:3 Changed 9 years ago by Thomas Güttler

Cc: hv@… added

FYI:

I wrote a small method (form_changed()) which compares the initial values of a form with
cleaned_data. It returns True if the form was changed.

http://www.djangosnippets.org/snippets/621/

May something like this can be used to implement the change history.

Changed 9 years ago by Alex Gaynor

Attachment: admin_log.diff added

This should work, brosner has said this should probably be in modelforms, I'm not familliar enough to work on those, however someone can use this work.

Changed 9 years ago by Alex Gaynor

Attachment: admin_log.2.diff added

Made it a bit more efficient

comment:4 Changed 9 years ago by Alex Gaynor

Has patch: set
Owner: changed from nobody to Alex Gaynor
Status: newassigned

comment:5 Changed 9 years ago by Karen Tracey <kmtracey@…>

I don't believe either of the existing patches covers logging changes to inline-edited objects. I'll attach what I have working that covers the inline-edited objects as well. Sorry I don't have more time to spend on this but maybe what I have will be useful in some way.

First, it makes the form responsible for reporting what has changed (I updated the logic I had originally used months ago to mirror what is currently in BaseForm's has_changed -- if this goes forward that logic should probably be consolidated in one place rather than being duplicated as I left it in the patch) via a changed_data property. This is something that could potentially be documented for general form users.

Similarly BaseModelFormSet is where the tracking of new/changed/deleted inline-edited objects goes. The formset stuff is not as clean as the single form case -- the information is generated as a side-effect of saving, which seems a bit iffy. Possibly the logic for determining what has been added/changed/deleted should be made independent of save -- save could then just use what added/changed/deleted information is reported by this split-out routine.

One thing this patch does that I really like is it suppresses saving of non-changed inline-edited objects. Thus if you've got upwards of a hundred inline-edited objects and tweak just one (typical for my particular use), you don't have to save all hundred or so, which seems like a win.

Changed 9 years ago by Karen Tracey <kmtracey@…>

Attachment: admin_log.3.diff added

Changed 9 years ago by Karen Tracey <kmtracey@…>

Attachment: admin_log.4.diff added

Fix error when adding 1st inline object, include quotes around object values like old code did (noticed during investigation of #6853)

comment:6 Changed 9 years ago by Matthijs Kooijman <matthijs@…>

Perhaps #4102 (which aims to keep track of changed fields with the model itself) could be of use here?

comment:7 Changed 9 years ago by Brian Rosner

Needs tests: set

Karen's patch is looking perfect. I can't see anything it is missing. Is there something I am? Otherwise just needs some tests and its RFC.

comment:8 Changed 9 years ago by Brian Rosner

Once again I don't fully read what Karen types ;) For purposes of inclusion of getting LogEntry working the patch is looking perfect.

Changed 9 years ago by Alex Gaynor

Attachment: admin_changed.diff added

Added some tests, there may be some test failures, I can't tell right now

Changed 9 years ago by Alex Gaynor

Attachment: admin_changed.2.diff added

Added some tests, there may be some test failures, I can't tell right now

Changed 9 years ago by Alex Gaynor

Attachment: 6117.diff added

This fixes the test failures from the previous patch

comment:9 Changed 9 years ago by Brian Rosner

Owner: changed from Alex Gaynor to Brian Rosner
Status: assignednew

Stealing this ticket. I am going to work on finishing this up and getting it committed.

comment:10 Changed 9 years ago by Brian Rosner

Status: newassigned

Oh and thanks for all your work Alex!

comment:11 Changed 9 years ago by Brian Rosner

Resolution: fixed
Status: assignedclosed

(In [7507]) newforms-admin: Fixed #6117 -- Implemented change history for the admin. This includes the ability to track changes on a newform. Model formsets now only return the changed/new objects saved. A big thanks to Karen Tracey and Alex Gaynor.

comment:12 Changed 8 years ago by Thomas Güttler

Cc: hv@… removed
Note: See TracTickets for help on using tickets.
Back to Top