Fixes #93 - Add 'changes_display_dict' property to 'LogEntry' model to display diff in a more human readable format #94

audiolion · 2017-01-29T16:40:23Z

'changes_display_dict' currently handles fields with choices, long textfields and charfields, and datetime fields.

Fields with choices display in the diff with their human readable text.
Textfields and Charfields longer than 140 characters are truncated with an ellipsis appended.
Datetime fields are rendered with the format 'Aug. 21, 2017, 02:25pm'
Support for Postgres's ArrayField and django-multiselectfield for those not on Postgres

A new kwarg was added to AuditlogModelRegistry called mapping_fields. The kwarg allows the user to map the fields in the model to a more human readable or intuitive name. If a field isn't mapped it will default to the name as defined in the model. Partial mapping is supported, all fields do not need to be mapped to use the feature.

Tests added for 'changes_display_dict'
Tests added for 'mapping_fields' property

Updated Documentation

Resolves #93

jjkester · 2017-02-09T19:13:59Z

Thanks for your contribution. Because the changes are bit more major than usual I'll mark this for version 0.5.0.

I do not have a lot of time to look through the changes now, so I'll come back to it later.

audiolion · 2017-02-16T03:12:04Z

so if you want I can break this PR up into a few separate changes, one would be to add the mapping fields attribute. This allow the diff to take a database defined name sku and map it to something more readable Product No.. This is an opt in feature (to provide field mappings in your auditlog register function) and we can include a boolean argument to the diff function to turn off the field mappings if you want to get the actual database field names.

Different PR will be for adding a mapping of choices attributes to human displayable text and other potential differences, where you would prefer to take a different approach, I will open a different issue.

audiolion · 2017-02-20T17:49:37Z

PR now only adds changes display dict functionality. If you are not in favor of this approach you can close the PR and I will just use my custom fork. It is my opinion that due to the need for using model meta attribute introspection to solve this problem, template tags are an inefficient solution for this problem.

From TwoScoops of Django Best Practices:

14.2.4 When to Write Template Tags

These days, we’re very cautious about adding new template tags. We consider two things before writing them:

Anything that causes a read/write of data might be better placed in a model or object method.

Since we implement a consistent naming standard across our projects, we can add an abstract base class model to our core.models module. Can a method or property in our project’s
abstract base class model do the same work as a custom template tag?

Implementing this as a templatetag would violate both bullets, it is both reading data and can be implemented in in a base class model

audiolion · 2017-02-20T19:56:41Z

With the updates you can uses changes_display_dict to get:

The current use of changes_dict does not map to human readable names and provides the actual database values like this:

The information from changes_dict is great for the programmer, what database field was changed, what database value was changed, however if you are displaying it to your end users or non-technical users who are using the admin site the human readable version helps a lot.

jjkester · 2017-02-28T15:14:31Z

OK, you convinced me. I have no time today to actually look at everything but I will take a look soon.

jjkester · 2017-03-27T17:43:54Z

.coveragerc

@@ -0,0 +1,13 @@
+[run]


I would like to have the coverage in another pull request.

jjkester · 2017-03-27T17:44:34Z

.gitignore

@@ -7,3 +7,4 @@
 .project
 .pydevproject
 local_settings.py
+.coverage


Occurrence of coverage

jjkester · 2017-03-27T17:49:31Z

README.md

@@ -27,3 +27,16 @@ Contribute
 ----------

 If you have great ideas for Auditlog, or if you like to improve something, feel free to fork this repository and/or create a pull request. I'm open for suggestions. If you like to discuss something with me (about Auditlog), please open an issue.
+
+Pull Request Guidelines


Can you put this in another pull request? Thanks!

jjkester · 2017-03-27T18:03:26Z

src/auditlog/models.py

@@ -229,6 +233,62 @@ def changes_str(self, colon=': ', arrow=smart_text(' \u2192 '), separator='; '):

        return separator.join(substrings)

+    @property
+    def changes_display_dict(self):


Can you inline-comment this? Than it is better understandable what it exactly does.

jjkester · 2017-03-27T18:06:52Z

src/auditlog/models.py

@@ -257,6 +317,7 @@ def __init__(self, pk_indexable=True, **kwargs):
        kwargs['content_type_field'] = 'content_type'
        super(AuditlogHistoryField, self).__init__(**kwargs)

+


Newline not needed here

yeah I think that was my editor adding it automatically on save ill remove

jjkester · 2017-03-27T18:07:51Z

src/auditlog_tests/models.py

+    )
+
+    status = models.CharField(max_length=1, choices=STATUS_CHOICES)
+    multiselect = MultiSelectField(max_length=3, choices=STATUS_CHOICES, max_choices=3)


Why are you explicitly testing with this third party library?

I use this library and wanted auditlog to be compatible with it

Right. If this is added to the tests I would also want the built-in PostgreSQL equivalent/replacement of it (https://docs.djangoproject.com/en/1.10/ref/contrib/postgres/fields/#arrayfield) since it is likely that people store lists that way as well.

yeah I am stuck on 1.8 so using it, I will add arrayfield support though, good idea!

jjkester · 2017-03-27T18:08:47Z

src/auditlog_tests/test_settings.py

@@ -19,7 +20,7 @@
 DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
-        'NAME': 'auditlog_tests.db',
+        'NAME': 'auditlog_test.db',


The 'app' is still called auditlog_tests, so why this change?

not needed change, ill revert

jjkester · 2017-05-22T19:07:30Z

The fixes do not seem to be pushed to this branch, so I cannot merge it. Can you do this so we can get the functionality in?

Fixes jazzband#93 - Add 'changes_display_dict' property to 'LogEntry' model to display diff in a more human readable format 'changes_display_dict' currently handles fields with choices, long textfields and charfields, and datetime fields. Fields with choices display in the diff with their human readable text. Textfields and Charfields longer than 140 characters are truncated with an ellipsis appended. Datetime fields are rendered with the format 'Aug. 21, 2017, 02:25pm' A new kwarg was added to 'AuditlogModelRegistry' called 'mapping_fields'. The kwarg allows the user to map the fields in the model to a more human readable or intuitive name. If a field isn't mapped it will default to the name as defined in the model. Partial mapping is supported, all fields do not need to be mapped to use the feature. Tests added for 'changes_display_dict' Tests added for 'mapping_fields' property Added python 3.5 support Updated Documentation Update .travis.yml to use requirements_test.txt as well in build

… dateutil's parser and would get formatted as datetime output. Introspection is used to see if the internal field type is DateTime, Date, or Time and then try to parse it and format it appropriately. This adds functionality in that DateTime, Date and Time each now have their separate formats.

of the 'mapping_fields' dictionary keys and values. The purpose of this dictionary is so that the LogEntry model's 'changes_display_dict' method can reverse a human readable field name returned from 'changes' so that it can do extra checks on that field. Without this we would get a FieldDoesNotExist error when trying to lookup fields on the model in the 'changes_display_dict' method so that we could introspect attributes about them when determining how to display human readable changes.

…f the model its associated to

codecov-io · 2017-09-06T19:31:11Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ac8638d). Click here to learn what that means.
The diff coverage is 94%.

@@            Coverage Diff            @@
##             master      #94   +/-   ##
=========================================
  Coverage          ?   69.15%           
=========================================
  Files             ?       18           
  Lines             ?      483           
  Branches          ?        0           
=========================================
  Hits              ?      334           
  Misses            ?      149           
  Partials          ?        0

Impacted Files	Coverage Δ
src/auditlog/registry.py	`86.27% <100%> (ø)`
src/auditlog/models.py	`81.21% <93.75%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac8638d...27ae05e. Read the comment docs.

Review comments have been addressed

audiolion · 2017-09-06T19:40:15Z

Hey @jjkester addressed all the review comments and got the diff coverage to 100%.

To support the request for Postgres's ArrayField I had to modify the test setup to run with multiple databases. In the process I found a bug where Auditlog saved LogEntry's to the default database always. I fixed this in 1aca8073 so that LogEntry's are saved to the database of the model they are attached to.

Also after this PR merges there will now be a base commit with coverage report to compare PR's to so we will get actual information from codecov reports.

…me fallback

jjkester · 2017-09-10T13:33:33Z

src/auditlog/models.py

+                from auditlog.registry import auditlog
+                model_fields = auditlog.get_model_fields(model._meta.model)
+                reversed_field = model_fields['reverse_mapping_fields'].get(field_name)
+                field = model._meta.get_field(reversed_field)


Again, a FieldDoesNotExist can be raised, for example when a model is changed.

Ok, with the update to mapping_fields not changing the diff.py output, I didn't need the reverse_mapping_fields. I have now removed that section, and in the case of FieldDoesNotExist fall back on the field_name auditlog has stored.

jjkester · 2017-09-10T13:35:39Z

src/auditlog/models.py

+                    if "DateTime" in field.get_internal_type():
+                        try:
+                            value = parser.parse(value)
+                            value = value.strftime("%b %d, %Y %I:%M %p")


I would rather see use of Django's l10n stuff for this, so that the format adapts to different settings.

Alright, modified to now rely on USE_L10N first and fallback on settings.DATETIME_FORMAT, settings.DATE_FORMAT, and settings.TIME_FORMAT which have defaults and can be set if the user desires.

Added test cases which demonstrate this.

jjkester · 2017-09-10T13:35:51Z

src/auditlog/models.py

+                    elif "Date" in field.get_internal_type():
+                        try:
+                            value = parser.parse(value)
+                            value = value.strftime("%b %d, %Y")


Again, l10n is preferred.

jjkester · 2017-09-10T13:36:00Z

src/auditlog/models.py

+                    elif "Time" in field.get_internal_type():
+                        try:
+                            value = parser.parse(value)
+                            value = value.strftime("%I:%M %p")


Again, l10n is preferred.

jjkester · 2017-09-10T13:40:13Z

src/auditlog/diff.py

@@ -135,7 +135,7 @@ def model_instance_diff(old, new):
        new_value = get_field_value(new, field)

        if old_value != new_value:
-            diff[field.name] = (smart_text(old_value), smart_text(new_value))
+            diff[model_fields['mapping_fields'].get(field.name, field.name)] = (smart_text(old_value), smart_text(new_value))


I would not use the mapping here. I think keeping the database representation as close to internal representation as possible is a good idea, especially because later on we are using database values to retrieve model fields. Changing the mapping would completely destroy that capability for the changed fields. The actual field names are less likely to change.

The mapping could be applied when the dictionary for display is built up (see my comment there for details).

Ah yes I agree, I reverted this change and now only change the name in the changes_display_dict property

jjkester · 2017-09-10T13:41:41Z

src/auditlog/models.py

+
+                    values_display.append(value)
+
+            changes_display_dict[field_name] = values_display


Instead of the field name we might use the mapping here. Also, we might consider going with the verbose name if present. So in order of importance: mapping - verbose name - name (the last is only guaranteed to be present).

Ok made this change, the order is mapping -> verbose name -> default verbose name. Django assigns a default verbose_name if none is provided so it is guaranteed to be always there.

If this isn’t given, Django will use a munged version of the class name: CamelCase becomes camel case.
ref

…ango's settings definitions.

…e changes_display_dict function. Remove usage of reverse_fields as it is no longer required. Default to field.verbose_name in the cases where there is no mapping_field, and if the field cannot be found default to the auditlog stored field_name

…ame in the auditlog diff itself

addressed concerns, awaiting new review

jjkester

You might change the docs a bit, otherwise it looks good

jjkester · 2017-09-13T07:28:52Z

docs/source/usage.rst

+      </table>
+    </div>
+
+If you want to display the changes in a more human readable format use the :py:class:`LogEntry`'s :py:attr:`changes_display_dict` instead. The :py:attr:`changes_display_dict` will translate ``choices`` fields into their human readable form, display timestamps in the form ``Jun. 31, 2017 12:55pm``, and truncate text greater than 140 characters to 140 characters with an ellipsis appended.


maybe be explicit about the l10n support

audiolion force-pushed the issue_93 branch from b538ea3 to 6b77fa0 Compare February 2, 2017 21:34

audiolion changed the title ~~Issue 93 - Display human readable text for CharFields with choices.~~ Fixes #93 - Add 'changes_display_dict' property to 'LogEntry' model to display diff in a more human readable format Feb 2, 2017

audiolion force-pushed the issue_93 branch 3 times, most recently from c0643d1 to 1468c6d Compare February 2, 2017 21:57

jjkester added this to the 0.5.0 milestone Feb 9, 2017

jjkester added the enhancement label Feb 9, 2017

audiolion force-pushed the issue_93 branch 2 times, most recently from e547ded to 14ef242 Compare February 20, 2017 17:44

jjkester previously requested changes Mar 27, 2017

View reviewed changes

audiolion mentioned this pull request Apr 24, 2017

Question: Extending this framework #114

Closed

audiolion added 3 commits September 6, 2017 12:47

audiolion force-pushed the issue_93 branch from 65c3383 to b80b876 Compare September 6, 2017 16:49

audiolion added 5 commits September 6, 2017 12:51

Revert .coveragerc addition

23cadae

Revert Pull Request Guideliens in README

8cc8f52

Revert changing test database name

1e8a7b4

Revert added newline

fd5069e

Add django-multiselectfield test dep, remove other req file

26283fc

audiolion force-pushed the issue_93 branch from 788a737 to 26283fc Compare September 6, 2017 17:29

audiolion added 3 commits September 6, 2017 13:30

Add psycopg2 test dep

cf66145

Add postgres testing database and router

609ec28

Add test for Postgres's ArrayField

a746302

audiolion added 5 commits September 6, 2017 13:56

Add history field to PostgresArrayFieldModel

17411f6

Add support for multiple databases. LogEntry saves to same database o…

1aca807

…f the model its associated to

If any literal evals fail default to None

6c23385

Add support for Postgres ArrayField in changes_display_dict

29f1431

Add tests for ArrayField

23236ce

Add comments to changes_display_dict function

89e1868

audiolion added 2 commits September 6, 2017 16:03

Add more tests to increase coverage

0a8fdf9

Remove unneeded if check, the default argument to get provides the sa…

61da565

…me fallback

audiolion force-pushed the issue_93 branch from e780a72 to 61da565 Compare September 6, 2017 20:36

audiolion mentioned this pull request Sep 6, 2017

Field verbose_name in log #135

Closed

jjkester previously requested changes Sep 10, 2017

View reviewed changes

audiolion added 6 commits September 11, 2017 12:04

Simplify date handling logic and format based upon localization or Dj…

8fc05e4

…ango's settings definitions.

Update tests to tests against django settings

9893d66

Add tests for when USE_L10N = True

6578780

Remove reverse_mapping_fields now that we aren't changing the field n…

26b3665

…ame in the auditlog diff itself

Update tests to test fallback on field.verbose_name

1930fb7

audiolion added 2 commits September 11, 2017 13:36

travis fix

380e144

Revert to old travis image while they are fixing issues with it

5fb6197

jjkester previously approved these changes Sep 13, 2017

View reviewed changes

Update usage.rst

55795e1

audiolion dismissed jjkester’s stale review via 55795e1 September 13, 2017 12:34

Update usage.rst

27ae05e

jjkester approved these changes Sep 13, 2017

View reviewed changes

audiolion merged commit 45760c6 into jazzband:master Sep 13, 2017

audiolion deleted the issue_93 branch September 13, 2017 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #93 - Add 'changes_display_dict' property to 'LogEntry' model to display diff in a more human readable format #94

Fixes #93 - Add 'changes_display_dict' property to 'LogEntry' model to display diff in a more human readable format #94

audiolion commented Jan 29, 2017 •

edited

Loading

jjkester commented Feb 9, 2017

audiolion commented Feb 16, 2017 •

edited

Loading

audiolion commented Feb 20, 2017 •

edited

Loading

14.2.4 When to Write Template Tags

audiolion commented Feb 20, 2017

jjkester commented Feb 28, 2017

jjkester Mar 27, 2017

jjkester Mar 27, 2017

jjkester Mar 27, 2017

jjkester Mar 27, 2017

audiolion Mar 27, 2017

jjkester Mar 27, 2017

audiolion Mar 27, 2017

jjkester Mar 27, 2017

audiolion Mar 27, 2017

jjkester Mar 27, 2017

audiolion Mar 27, 2017

jjkester Mar 27, 2017

audiolion Mar 27, 2017

jjkester commented May 22, 2017

codecov-io commented Sep 6, 2017 •

edited

Loading

audiolion commented Sep 6, 2017 •

edited

Loading

jjkester Sep 10, 2017

audiolion Sep 11, 2017

jjkester Sep 10, 2017

audiolion Sep 11, 2017 •

edited

Loading

jjkester Sep 10, 2017

jjkester Sep 10, 2017

jjkester Sep 10, 2017

audiolion Sep 11, 2017

jjkester Sep 10, 2017

audiolion Sep 11, 2017

jjkester left a comment

jjkester Sep 13, 2017

		@@ -257,6 +317,7 @@ def __init__(self, pk_indexable=True, **kwargs):
		kwargs['content_type_field'] = 'content_type'
		super(AuditlogHistoryField, self).__init__(**kwargs)


		values_display.append(value)

		changes_display_dict[field_name] = values_display

Fixes #93 - Add 'changes_display_dict' property to 'LogEntry' model to display diff in a more human readable format #94

Fixes #93 - Add 'changes_display_dict' property to 'LogEntry' model to display diff in a more human readable format #94

Conversation

audiolion commented Jan 29, 2017 • edited Loading

jjkester commented Feb 9, 2017

audiolion commented Feb 16, 2017 • edited Loading

audiolion commented Feb 20, 2017 • edited Loading

14.2.4 When to Write Template Tags

audiolion commented Feb 20, 2017

jjkester commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjkester commented May 22, 2017

codecov-io commented Sep 6, 2017 • edited Loading

Codecov Report

audiolion commented Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audiolion Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjkester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audiolion commented Jan 29, 2017 •

edited

Loading

audiolion commented Feb 16, 2017 •

edited

Loading

audiolion commented Feb 20, 2017 •

edited

Loading

codecov-io commented Sep 6, 2017 •

edited

Loading

audiolion commented Sep 6, 2017 •

edited

Loading

audiolion Sep 11, 2017 •

edited

Loading