Opened 10 years ago

Closed 9 years ago

Last modified 9 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 10 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 10 years ago.
Made it a bit more efficient
admin_log.3.diff (5.5 KB) - added by Karen Tracey <kmtracey@…> 10 years ago.
admin_log.4.diff (5.6 KB) - added by Karen Tracey <kmtracey@…> 10 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 10 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 10 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 10 years ago.
This fixes the test failures from the previous patch

Download all attachments as: .zip

Change History (19)

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

comment:2 Changed 10 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 10 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 10 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 10 years ago by Alex Gaynor

Attachment: admin_log.2.diff added

Made it a bit more efficient

comment:4 Changed 10 years ago by Alex Gaynor

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

comment:5 Changed 10 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 10 years ago by Karen Tracey <kmtracey@…>

Attachment: admin_log.3.diff added

Changed 10 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 10 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 10 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 10 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 10 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 10 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 10 years ago by Alex Gaynor

Attachment: 6117.diff added

This fixes the test failures from the previous patch

comment:9 Changed 10 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 10 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 9 years ago by Thomas Güttler

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