feat: allow reuse of status indicators

fsbraun commented 1 year ago

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

[x] I have opened this pull request against master
[x] I have added or modified the tests when changing logic
[x] I have followed the conventional commits guidelines to add meaningful information into the changelog
[x] 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[bot] commented 1 year ago

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%)`	:arrow_up:
djangocms_versioning/indicators.py	`91.07% <92.85%> (+11.07%)`	:arrow_up:
djangocms_versioning/cms_config.py	`81.77% <100.00%> (+1.17%)`	:arrow_up:
djangocms_versioning/cms_toolbars.py	`95.95% <100.00%> (ø)`
djangocms_versioning/datastructures.py	`95.40% <100.00%> (-0.16%)`	:arrow_down:
djangocms_versioning/helpers.py	`91.48% <100.00%> (+0.37%)`	:arrow_up:
djangocms_versioning/managers.py	`85.24% <100.00%> (+2.55%)`	:arrow_up:
djangocms_versioning/test_utils/blogpost/admin.py	`100.00% <100.00%> (ø)`
djangocms_versioning/versionables.py	`100.00% <100.00%> (+8.33%)`	:arrow_up:

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

fsbraun commented 1 year ago

@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.

fsbraun commented 1 year ago

@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.)

fsbraun commented 1 year ago

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

django-cms / djangocms-versioning