feat: allow reuse of status indicators #319

fsbraun · 2023-02-12T12:08:22Z

Description

This PR refactors the indicators.py setup to allow using indicators outside the PageTree (i.e. other versioned models). Existing functionality does not change.

The indicator code now works for all versioned object types
The indicator mixing for the page content admin has been moved into the page content admin
Some pagecontent-based helper functions have been generalized for all versioned models
To improve db access a latest_content() has been added to the admin_manager query set. It respects prefetched content objects reducing db accesses
Documentation is updated
Tests for content indicators for non-page content objects included

Usage

The status indicator for versioning can be added to a ModelAdmin by:

    from djangocms_versioning.admin import StateIndicatorMixin

    class MyModelAdmin(StateIndicatorMixin, admin.Admin):
        # Add indicator column to list_items
         list_items = [..., "status_indicator", ...]

Remarks:

Using the mixin will load the cms.pagetree.css (not the js) from the core into the admin form
Additionally, the indicators also work with ExtendedVersionAdminMixin.
The new ExtendedIndicatorVersionAdminMixin combines state indicators and the extended versioning admin.
Test are added
Documentation is updated

Example: djangocms-alias

This is the result of relpacing ExtendedVersionAdminMixin with ExtendedIndicatorVersionAdminMixin for djangocms-alias:

Checklist

I have opened this pull request against master
I have added or modified the tests when changing logic
I have followed the conventional commits guidelines to add meaningful information into the changelog
I have read the contribution guidelines and I have joined #workgroup-pr-review on
Slack to find a “pr review buddy” who is going to review my pull request.

codecov · 2023-02-12T12:11:41Z

Codecov Report

Merging #319 (f5c5fcb) into master (7e41d8a) will increase coverage by 0.53%.
The diff coverage is 92.90%.

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   89.99%   90.53%   +0.53%     
==========================================
  Files          68       68              
  Lines        2259     2324      +65     
  Branches      301      314      +13     
==========================================
+ Hits         2033     2104      +71     
+ Misses        171      165       -6     
  Partials       55       55

Impacted Files	Coverage Δ
djangocms_versioning/forms.py	`100.00% <ø> (ø)`
djangocms_versioning/admin.py	`89.13% <85.71%> (+0.37%)`	⬆️
djangocms_versioning/indicators.py	`91.07% <92.85%> (+11.07%)`	⬆️
djangocms_versioning/cms_config.py	`81.77% <100.00%> (+1.17%)`	⬆️
djangocms_versioning/cms_toolbars.py	`95.95% <100.00%> (ø)`
djangocms_versioning/datastructures.py	`95.40% <100.00%> (-0.16%)`	⬇️
djangocms_versioning/helpers.py	`91.48% <100.00%> (+0.37%)`	⬆️
djangocms_versioning/managers.py	`85.24% <100.00%> (+2.55%)`	⬆️
djangocms_versioning/test_utils/blogpost/admin.py	`100.00% <100.00%> (ø)`
djangocms_versioning/versionables.py	`100.00% <100.00%> (+8.33%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

djangocms_versioning/test_utils/blogpost/admin.py

 from djangocms_versioning.test_utils.blogpost import models


-class BlogContentAdmin(ExtendedVersionAdminMixin, admin.ModelAdmin):
-    pass
+class BlogContentAdmin(StateIndicatorMixin, ExtendedVersionAdminMixin, admin.ModelAdmin):


djangocms_versioning/test_utils/blogpost/admin.py



-admin.site.register(models.BlogPost)
+class BlogPostAdmin(StateIndicatorMixin, ExtendedVersionAdminMixin, admin.ModelAdmin):


djangocms_versioning/admin.py

@@ -326,6 +407,14 @@
        return list_display


+class ExtendedIndicatorVersionAdminMixin(StateIndicatorMixin, ExtendedVersionAdminMixin):


djangocms_versioning/admin.py

fsbraun · 2023-02-21T15:11:57Z

@Aiky30 I've updated the PR.

I do not expect additional backwards incompatible behavior. This version of versioning will not work with Django CMS 4.0 anyways since it does not use monkey patching.

All elements were tested, but only indirectly. I added tests to explicitly test especially the new query sets.

djangocms_versioning/helpers.py

Aiky30 · 2023-03-02T18:43:15Z

djangocms_versioning/admin.py

+        js = ("admin/js/jquery.init.js", "djangocms_versioning/js/indicators.js",)
+        # css for indicators and context menu
+        css = {
+            "all": (static_with_version("cms/css/cms.pagetree.css"),),


A ticket to create for a future change in the core and here is to separate the indicator styling out of this file.

djangocms_versioning/admin.py

Aiky30 · 2023-03-02T18:49:10Z

djangocms_versioning/admin.py

+         """
+        grouping_filters = {}
+        for field in versionable.extra_grouping_fields:
+            if hasattr(self, f"get_{field}_from_request"):


I'm not seeing unit test coverage for this new condition that checks that the get_field_form_request. Is it covered by another test?

djangocms_versioning/helpers.py

tests/test_indicators.py

Aiky30 · 2023-03-02T19:17:40Z

tests/test_indicators.py

+        self.assertEqual(content.versions.first(), version1)
+
+        # Now archive
+        version1.archive(user=self.get_superuser())


Not that important but each check could have been a separate test in the Class, with the version being defined in setup. would have been able to keep the AAA test pattern that's used elsewhere then too.

Changed it.

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

djangocms_versioning/helpers.py

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

djangocms_versioning/admin.py

    get_preview_url,
    proxy_model,
    version_list_url,
 )
+from .indicators import content_indicator, content_indicator_menu


fsbraun · 2023-03-06T09:43:28Z

@Aiky30 OK, I'm ready for round three! Thanks for asking for the test for get_{field}_from_request. The test (now added) revealed that I was looking for that method in the ChangeList class while it should be in the model admin class. 🙌

The only open point is the import of cms.pagetree.css which I believe is ok:

cms.pagetree.css contains: icons.scss, dropdown.scss, legend.scss, and node-state.scss which are needed by this PR
cms.pagetree.css also contains tree.scss which is not needed by this PR.

I figured that css.pagetree.css will be available in the browser cache with a high probability, and there is little chance of erroneous cross-styling as long as you do not have .jstree or .cms-tree-item-xxx or .cms-pagetree-xxx in your changelist admin templates. (If you have to have you will not be able to use the mixin.)

Aiky30

Looks good and great test coverage. Good job. Thanks for taking the time to consider and act on the comments. :-)

fsbraun · 2023-03-11T08:16:21Z

Thanks for talking the time to look through this. I appreciate the comments (and the additional coverage prevented a bug 🙂).

fsbraun added 14 commits February 9, 2023 12:40

Allow page indicators for any versioned model

aa97881

MySQL compatibility

5e29905

Fix queryset initialization

679e2fc

fix isort

de56ba1

Generalize get_latest_admin_viewable_page_content

7f18122

fix flake8

1de2317

Fix: grouper can be model or instance

c948e5f

fix: identify correct inverse relation

0125c2a

Remove IndicatorMixin

bcf56b2

Add tests

5570b58

no message

5ca7d85

More flexible list_display option

60fe7f3

Improve back functionality of get views

9939a1b

Fix isort

c0346ab

github-advanced-security bot found potential problems Feb 12, 2023

View reviewed changes

djangocms_versioning/test_utils/blogpost/admin.py Fixed Show fixed Hide fixed

djangocms_versioning/test_utils/blogpost/admin.py Fixed Show fixed Hide fixed

fsbraun added 6 commits February 12, 2023 14:09

Add one more test, fix doc inconsistency

43c6fc4

fix coverage

c14bc6d

Let get_list_display return tuple

9c70b64

Fix isort

61107fe

Fix test bugs

9d5a90b

Remove empty line

a49a141

fsbraun requested review from Aiky30 and marksweb February 13, 2023 11:41

fsbraun added 3 commits February 13, 2023 13:14

Fix: Add MediaDefiningClass meta classes

1d6740f

Refactor for more consistent api

8daf0c9

Update tests

f2b4c3e

github-advanced-security bot found potential problems Feb 13, 2023

View reviewed changes

fsbraun added 2 commits February 13, 2023 15:41

Fix 2 missing renames

4f186db

Remove spourious import cycle

cb9cf6d

fsbraun added 3 commits February 21, 2023 15:21

Update release notes

6e793e6

Fix: Clarify docs (page tree as example)

6fb42e7

Update docs

ffe1f4c

fsbraun added 2 commits February 22, 2023 12:28

Merge branch 'master' into feat/code-reusability

921367f

Merge branch 'master' into feat/code-reusability

8d98775

Aiky30 reviewed Mar 2, 2023

View reviewed changes

fsbraun and others added 4 commits March 2, 2023 20:36

Update djangocms_versioning/helpers.py

d193a0a

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

Update tests/test_admin.py

f31d501

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

Update tests/test_indicators.py

033836b

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

Merge branch 'master' into feat/code-reusability

72bd397

github-advanced-security bot found potential problems Mar 2, 2023

View reviewed changes

djangocms_versioning/helpers.py Fixed Show fixed Hide fixed

fsbraun and others added 6 commits March 2, 2023 20:53

fix indentation

8f52295

Update tests/test_indicators.py

6405b45

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

Update tests/test_indicators.py

e7bea5c

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

Update tests/test_indicators.py

62e2cb1

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

Update tests/test_admin.py

b2ce48c

Co-authored-by: Andrew Aikman <Aiky30@users.noreply.github.com>

Move indicator names to constants, add tests for versionables module

9eabf14

github-advanced-security bot found potential problems Mar 2, 2023

View reviewed changes

djangocms_versioning/admin.py Fixed Show fixed Hide fixed

fsbraun added 3 commits March 2, 2023 21:41

fix flake8

c379d74

fix isort

50cc9e8

simpler imports

0c2baa7

github-advanced-security bot found potential problems Mar 2, 2023

View reviewed changes

djangocms_versioning/admin.py

get_preview_url,

proxy_model,

version_list_url,

)

from .indicators import content_indicator, content_indicator_menu

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [djangocms_versioning.indicators](1) begins an import cycle.

Fix: get_{field}_from_request now needs to be present in model admin

ca30948

Aiky30 approved these changes Mar 10, 2023

View reviewed changes

fix 2 typos

f5c5fcb

fsbraun merged commit 1e8a48c into django-cms:master Mar 11, 2023

fsbraun deleted the feat/code-reusability branch March 15, 2023 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow reuse of status indicators #319

feat: allow reuse of status indicators #319

fsbraun commented Feb 12, 2023 •

edited

Loading

codecov bot commented Feb 12, 2023 •

edited

Loading

fsbraun commented Feb 21, 2023

Aiky30 Mar 2, 2023

Aiky30 Mar 2, 2023

Aiky30 Mar 2, 2023

fsbraun Mar 6, 2023

fsbraun commented Mar 6, 2023

Aiky30 left a comment

fsbraun commented Mar 11, 2023



		admin.site.register(models.BlogPost)
		class BlogPostAdmin(StateIndicatorMixin, ExtendedVersionAdminMixin, admin.ModelAdmin):

		@@ -326,6 +407,14 @@
		return list_display


		class ExtendedIndicatorVersionAdminMixin(StateIndicatorMixin, ExtendedVersionAdminMixin):

feat: allow reuse of status indicators #319

feat: allow reuse of status indicators #319

Conversation

fsbraun commented Feb 12, 2023 • edited Loading

Description

Usage

Example: djangocms-alias

Checklist

codecov bot commented Feb 12, 2023 • edited Loading

Codecov Report

fsbraun commented Feb 21, 2023

Aiky30 Mar 2, 2023

Choose a reason for hiding this comment

Aiky30 Mar 2, 2023

Choose a reason for hiding this comment

Aiky30 Mar 2, 2023

Choose a reason for hiding this comment

fsbraun Mar 6, 2023

Choose a reason for hiding this comment

fsbraun commented Mar 6, 2023

Aiky30 left a comment

Choose a reason for hiding this comment

fsbraun commented Mar 11, 2023

fsbraun commented Feb 12, 2023 •

edited

Loading

codecov bot commented Feb 12, 2023 •

edited

Loading