Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#6117 closed (fixed)

newforms-admin change history needs to be implemented

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

Download all attachments as: .zip

Change History (19)

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 8 years ago by brosner

  • Keywords nfa-blocker added
  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 7 years ago by guettli

  • 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 7 years ago by Alex

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

Made it a bit more efficient

comment:4 Changed 7 years ago by Alex

  • Has patch set
  • Owner changed from nobody to Alex
  • Status changed from new to assigned

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

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

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

comment:6 Changed 7 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 7 years ago by brosner

  • 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 7 years ago by brosner

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

Changed 7 years ago by Alex

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

Changed 7 years ago by Alex

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

Changed 7 years ago by Alex

This fixes the test failures from the previous patch

comment:9 Changed 7 years ago by brosner

  • Owner changed from Alex to brosner
  • Status changed from assigned to new

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

comment:10 Changed 7 years ago by brosner

  • Status changed from new to assigned

Oh and thanks for all your work Alex!

comment:11 Changed 7 years ago by brosner

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 7 years ago by guettli

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