Log operations #6419

Aiky30 · 2018-06-19T16:16:37Z

Summary

The purpose of this PR is to get early feedback on how the logging has been implemented and any ideas / changes that Paulo may require.

Logs are all collected in the cms/utils/log_operations to keep the messages and contents consistent across all operations. It May seem repetitive but allows each log to be unique for different circumstances. The method names aim to make the log being created clear and concise.

Issues with the existing solution:
- A log entry isn't created when a page is created using the new page wizard.
- Difficult to find the log page creation as they are scattered. The existing implementation,uses the LogEntry model directly, others use the ModelAdmin Implementation. There should be one way where possible IMO.

Design Considerations:
- Allow external plugins to integrate with the CMS logger i.e. custom logs, may be useful if the LogEntry method is replaced for a different logging facility??
- A log helper method "create_log" that uses LogEntry and allows the removal of LogEntry at a later date.
- A page move event is recorded as CHANGE with no message. A "Moved." message has been manually added. There is a comment in the code to make this clear.
- Django admin keeps all logs in one area rather than spreading them out and potentially having them set inconsistently and scattered in multiple files.

Links to related discussion

Private documentation

…sts to accept the message change

…and the move page log

…on spews the correct format

…orking is very ugly

coveralls · 2018-06-19T16:47:19Z

Coverage remained the same at 78.189% when pulling 2c0a1c6 on Aiky30:log-operations into 61150ed on divio:release/4.0.x.

czpython · 2018-06-19T17:41:38Z

cms/admin/forms.py

@@ -5,6 +5,9 @@
 from django.contrib.auth.models import Permission
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.sites.models import Site
+from django.contrib.admin.utils import (


single line

czpython · 2018-06-19T17:44:53Z

cms/admin/pageadmin.py

@@ -1221,6 +1246,7 @@ def unpublish(self, request, page_id, language):
            page.unpublish(language)
            message = _('The %(language)s page "%(page)s" was successfully unpublished') % {
                'language': language_name, 'page': page}
+


please avoid cosmetic changes unrelated to pr

Understood. I'll rectify.

czpython · 2018-06-19T17:51:23Z

cms/utils/log_operations.py

@@ -0,0 +1,122 @@
+# -*- coding: utf-8 -*-


rename to log_entries

czpython · 2018-06-19T17:51:39Z

cms/utils/log_operations.py

+"""
+
+
+def create_log(user_id, content_type_id, object_id, object_repr, action_flag, change_message):


rename to create_log_entry

czpython · 2018-06-19T17:54:54Z

cms/admin/pageadmin.py

@@ -75,6 +75,7 @@
 from cms.utils.admin import jsonify_request
 from cms.utils.conf import get_cms_setting
 from cms.utils.urlutils import admin_reverse
+from cms.utils.log_operations import log_page_change, log_page_move, log_page_delete


I'm foreseeing issues with the implementations of title, plugins, etc because this is too tied to the admin.
As a result, I think it might be a better approach to use signal handlers and listen for the post_obj_operation signals for pages and post_placeholder_operation signal for placeholders and plugins.

I suggest to create a module called log_entries.py in cms/signals/
and there add all the necessary handlers.

So this won't really change much in your current implementation, other than where the log entry is created.

All of this is fine but falls short for title changes as the signals for these are attached to a page change. No signals are fired specifically to title. One way to achieve this would be to catch a page edit and check what was being changed and log dependant on that. It's not a clean solution.

czpython · 2018-06-19T17:56:20Z

cms/utils/log_operations.py

+Page logs
+"""
+
+


lots of repetition with the page log functions, also I don't think we should have one for each type of entry.
With the signal handler approach I suggested above, maybe create a page specific handler which receives the operation type and then map operation types to action flags.

Aiky30 · 2018-06-20T16:04:33Z

cms/tests/test_log_operations.py

+        message = '[{"added": {}}]'
+        self.assertEqual(message, log_entry.change_message)
+        message = 'Added.'
+        self.assertEqual(message, log_entry.get_change_message())


@czpython how far does the removal of LogEntry dependance need to go i.e.the test for get_change_message is specific to how the admin uses the multi lingual json objects in the change_message field. I sthis test redundant and does that also mean any auto generated messages are also redundant. I'm confused at just how far away this task needs to move away from the Admin LogEntry model.

Good point. We don't need to test get_change_message.

…approach.

…operations to page with a CHANGE flag

…e as the source object

… deleted handlers for the Page model.

…reations

… intended

czpython · 2018-06-26T20:29:20Z

cms/admin/forms.py

+            token=operation_token,
+            obj=new_page,
+        )
+


remove newline

czpython · 2018-06-26T20:29:33Z

cms/admin/forms.py

@@ -443,6 +467,12 @@ def clean(self):
        return data

    def save(self, commit=True):
+


remove newline

czpython · 2018-06-26T20:29:35Z

cms/admin/forms.py

+            request=self.request,
+            operation=CHANGE_PAGE,
+        )
+


remove newline

czpython · 2018-06-26T20:30:09Z

cms/admin/forms.py

+            request=self.request,
+            operation=CHANGE_PAGE,
+        )
+
        data = self.cleaned_data


I would move data = self.cleaned_data above operation_token

czpython · 2018-06-26T20:30:25Z

cms/admin/forms.py

+            token=operation_token,
+            obj=cms_page,
+        )
+


remove newline

czpython · 2018-06-26T20:53:34Z

cms/signals/log_entries.py

+    Create a log for the correct placeholder operation type
+    """
+
+    request = kwargs.pop('request')


use .get()

czpython · 2018-06-26T20:53:38Z

cms/signals/log_entries.py

+        content_type_id = ContentType.objects.get_for_model(page).pk
+        object_id = page.pk
+        object_repr = str(page)
+


remove newline

czpython · 2018-06-26T20:53:53Z

cms/signals/log_entries.py

+        object_id = page.pk
+        object_repr = str(page)
+
+        create_log_entry(user_id, content_type_id, object_id, object_repr, operation_handler['flag'], operation_handler['message'])


same as above. kwargs with each arg in one line

czpython · 2018-06-26T20:54:12Z

cms/signals/log_entries.py

+        operation_handler = _placeholder_operations_map[operation_type]
+        user_id = request.user.pk
+        placeholder = kwargs.pop(operation_handler['placeholder_kwarg'])
+        page = placeholder.page


will need to check that placeholder actually has page

czpython · 2018-06-26T20:57:58Z

cms/tests/test_log_entries.py

+from django.forms.models import model_to_dict
+from django.utils.translation import ugettext_lazy as _
+
+from cms.api import create_page, add_plugin, create_title


import order

Aiky30

I think I can see your style of coding now so new line comments should be few and far between in the future. :-)

Aiky30 · 2018-06-27T08:42:48Z

cms/admin/pageadmin.py

-        )
-        return token
+            operation=operation,
+            sender=self.__class__,


I had initially tried self.model but it wasn't available for some reason. It's possible that one failed so I removed them all for consistency. I'll add them back and test.

Aiky30 · 2018-06-27T10:30:12Z

cms/signals/log_entries.py

+}
+
+
+def create_log_entry(user_id, content_type_id, object_id, object_repr, action_flag, change_message):


Yup. works.

Aiky30 · 2018-06-27T10:32:13Z

cms/signals/log_entries.py

+    Create a log for the correct page operation type
+    """
+
+    request = kwargs.pop('request')


I got this from previous examples of how Django CMS fetched the kwargs. A search in all cms files will show all of the examples of "kwargs.pop". I try to follow how the app is already written when possible. :-). I'll update mine to get. ;-)

Aiky30 · 2018-06-27T10:45:09Z

cms/signals/log_entries.py

+    operation_type = kwargs.pop('operation')
+    obj = kwargs.pop('obj')
+
+    if operation_type in _page_operations_map:


I've created a private function to fix this that checks for if the supplied object is a Page or PageType instance. I'm thinking that checking for just Page is OK though. What is the reasoning for an additional PageType check?

Aiky30 · 2018-06-27T12:03:21Z

cms/admin/pageadmin.py

-        )
-        return token
+            operation=operation,
+            sender=self.__class__,


I hope that I've interpreted your comment correctly. I've removed the private class methods and just used the functions directly. This required changing a lot of the calls to fire a signal.

…ase/4.0.x

…n file

…emoved the spoofed version

Aiky30 · 2018-06-27T14:44:25Z

@czpython
All checks are passing and I have made all of the changes that you requested. Please let me know if you need me to do anymore. I have also tested the signals by actually playing with the UI too rather than relying purely on the tests. :-)

czpython · 2018-06-27T22:22:50Z

cms/admin/pageadmin.py

@@ -480,10 +470,12 @@ def delete_view(self, request, object_id, extra_context=None):
        return self.render_delete_form(request, context)

    def delete_model(self, request, obj):
-        operation_token = self._send_pre_page_operation(
-            request,
+


remove newline

czpython · 2018-06-27T22:23:35Z

cms/admin/pageadmin.py

)
+

remove newline

czpython · 2018-06-27T22:25:20Z

cms/signals/log_entries.py

@@ -0,0 +1,136 @@
+from django.contrib.admin.models import LogEntry, ADDITION, CHANGE, DELETION


Missing this

czpython · 2018-06-27T22:26:28Z

cms/signals/log_entries.py

+        user_id = request.user.pk
+        content_type_id = ContentType.objects.get_for_model(obj).pk
+        object_id = obj.pk
+        object_repr = str(obj)


this can be passed directly without declaring var

czpython · 2018-06-27T22:26:30Z

cms/signals/log_entries.py

+        operation_handler = _page_operations_map[operation_type]
+        user_id = request.user.pk
+        content_type_id = ContentType.objects.get_for_model(obj).pk
+        object_id = obj.pk


this can be passed directly without declaring var

czpython · 2018-06-27T22:26:35Z

cms/signals/log_entries.py

+    # Check that we have instructions for the operation and an instance of Page to link to in the log
+    if operation_type in _page_operations_map and _is_valid_page_instance(obj):
+        operation_handler = _page_operations_map[operation_type]
+        user_id = request.user.pk


this can be passed directly without declaring var

czpython · 2018-06-27T22:26:45Z

cms/signals/log_entries.py

+    operation_type = kwargs.get('operation')
+    obj = kwargs.get('obj')
+
+    # Check that we have instructions for the operation and an instance of Page to link to in the log


break this into two lines

czpython · 2018-06-27T22:27:20Z

cms/signals/log_entries.py

+    """
+    Create a log for the correct page operation type
+    """
+


avoid newlines after doc strings

czpython · 2018-06-27T22:27:37Z

cms/signals/log_entries.py

+        # Check that we have an instance of Page to link to in the log
+        if _is_valid_page_instance(page):
+            content_type_id = ContentType.objects.get_for_model(page).pk
+            object_id = page.pk


see above about passing these directly

…the test

Aiky30

All done as per the request.

…for both placeholder and page

Andrew Aikman added 12 commits June 15, 2018 16:54

Logging helper, page operation logs and page operation tests implemented

c393d73

Amended the logs to contain the correct action flag

8309bac

Added tests for the action flag, changed the page log messages and te…

3365d4f

…sts to accept the message change

Updated the messages and tests to cover more of the log entry fields …

e18af96

…and the move page log

Added new check for get_change_message to ensure that the view functi…

41af0c5

…on spews the correct format

Cleaning the comments in the code

8ca4740

All tests now passing although the method of getting the page admin w…

e39fbb5

…orking is very ugly

Cleaned comments and Todo message

d7677df

Cleaned out and organised better for pep 8 compliance

721948e

Cleaned out comments

d92ba32

logging page creation shared tests renamed

8b79b72

Clean code to pass py flakes tests

27bb832

czpython suggested changes Jun 19, 2018

View reviewed changes

Aiky30 commented Jun 20, 2018

View reviewed changes

czpython self-assigned this Jun 20, 2018

czpython added the status: work in progress label Jun 20, 2018

czpython added this to the 4.0.0 milestone Jun 20, 2018

czpython added status: accepted kind: enhancement labels Jun 20, 2018

czpython changed the title ~~WIP - Log operations~~ Log operations Jun 20, 2018

Andrew Aikman added 9 commits June 21, 2018 17:14

Added various changes to the logging including changing to a signals …

2c0df27

…approach.

committing progress for discussion

801a901

Changed the logging message format and the object of all placeholder …

32a91db

…operations to page with a CHANGE flag

Removed the log creation from the form. Amended logs to treat the pag…

322181d

…e as the source object

Added all tests, disabled the standard Admin log created, changed and…

467ea74

… deleted handlers for the Page model.

Comment the disabled log entry overrides and remove Manual LogEntry c…

b0d90be

…reations

Reverse changes to the form file

84c6aa5

Prepare for the form signal handler via operations. Failing tests.

6ccfb02

Added fake request to continue dev. Amended tests and code to work as…

d707ce8

… intended

Andrew Aikman added 5 commits June 26, 2018 15:49

Amended the logs to fire for edit title fieldgit status

c5c032d

Add a page changed flag for when fields on page are changed

9bd9902

Repair changes made to other files

ccc6354

Removed unused plugin

e741e04

Remove space that I've added

e723bd1

czpython suggested changes Jun 26, 2018

View reviewed changes

Amendments from PR feedback

81f9024

Aiky30 commented Jun 27, 2018

View reviewed changes

Andrew Aikman added 5 commits June 27, 2018 13:55

Merge branch 'release/4.0.x' of github.com:divio/django-cms into rele…

9f0a770

…ase/4.0.x

Repair merge of latest test failure

6030a71

Removed failing test that checks for publishing logs in the test_admi…

b82416d

…n file

Amend the admin form to send through the correct request object and r…

8bed9d0

…emoved the spoofed version

Removed redundnant LogEntry import from test_admin

c192dce

czpython added status: ready for review and removed status: work in progress labels Jun 27, 2018

czpython suggested changes Jun 28, 2018

View reviewed changes

Andrew Aikman and others added 4 commits June 28, 2018 09:33

Format the code as per Paulos PR feedback

89b0249

fixed failing test due to cleaning too far

4e4d331

cosmetics

b57802c

Updated all of the test docstrings to better describe the purpose of …

3b4e002

…the test

Aiky30 commented Jun 28, 2018

View reviewed changes

czpython approved these changes Jun 28, 2018

View reviewed changes

Andrew Aikman added 3 commits June 29, 2018 11:44

Add tests that the correct user is attached to each log

4bf6ce9

Amended log entry tests by ensuring the correct users are configured …

153a60b

…for both placeholder and page

Changed the users id to pk on the tests£

2c0a1c6

czpython merged commit 0394153 into django-cms:release/4.0.x Jul 2, 2018

		"""


		def create_log(user_id, content_type_id, object_id, object_repr, action_flag, change_message):

		@@ -443,6 +467,12 @@ def clean(self):
		return data

		def save(self, commit=True):

		}


		def create_log_entry(user_id, content_type_id, object_id, object_repr, action_flag, change_message):

		@@ -0,0 +1,136 @@
		from django.contrib.admin.models import LogEntry, ADDITION, CHANGE, DELETION

Log operations #6419

Log operations #6419

Conversation

Aiky30 commented Jun 19, 2018

Summary

Links to related discussion

coveralls commented Jun 19, 2018 • edited

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

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

Aiky30 left a 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

Aiky30 commented Jun 27, 2018

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

Aiky30 left a comment

Choose a reason for hiding this comment

coveralls commented Jun 19, 2018 •

edited