django-cms / djangocms-moderation

Other
13 stars 20 forks source link

added multiselect and add items to collection in admin #273

Closed vipulnarang95 closed 22 hours ago

vipulnarang95 commented 1 week ago

Description

Feature added: Add items to collection action added for all moderated models in cms_config

Related resources

Checklist

vipulnarang95 commented 1 week ago

@fsbraun Please check and review

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.05%. Comparing base (79aaae4) to head (9318fc5). Report is 58 commits behind head on master.

Files Patch % Lines
djangocms_moderation/cms_config.py 90.00% 0 Missing and 1 partial :warning:

:exclamation: There is a different number of reports uploaded between BASE (79aaae4) and HEAD (9318fc5). Click for more details.

HEAD has 2 uploads less than BASE | Flag | BASE (79aaae4) | HEAD (9318fc5) | |------|------|------| ||12|10|
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #273 +/- ## ========================================== - Coverage 84.19% 79.05% -5.15% ========================================== Files 23 40 +17 Lines 1740 1862 +122 Branches 282 260 -22 ========================================== + Hits 1465 1472 +7 - Misses 245 359 +114 - Partials 30 31 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fsbraun commented 1 week ago

Looks good. Can you add some tests? What happens if there are published, unpublished or archived objects selected when the action is run?

fsbraun commented 1 week ago

Actually, this is a nice example of the power of cms_config.

vipulnarang95 commented 1 week ago

@fsbraun Tests are already there: https://github.com/django-cms/djangocms-moderation/blob/master/tests/test_forms.py#L146 Do you want me to add some specific?

fsbraun commented 1 week ago

Test adding a mixture of objects with different version states. I would expect a user message that some items could not be added to the collection since they are not drafts, or are locked.

Also, the test should not use the form directly, but add_items_to_collection instead.

vipulnarang95 commented 4 days ago

Hi @fsbraun , I was trying adding test cases accordingly but after writing initial test cases and debugging, found that add_items_to_collection only checks if version_ids are available it will return a redirect url to moderation screen. https://github.com/django-cms/djangocms-moderation/blob/master/djangocms_moderation/admin_actions.py#L143 The else section only gets called when nothing is selected. Can you please check once?

vipulnarang95 commented 1 day ago

Hi @fsbraun can you also check my comment above?

fsbraun commented 1 day ago

@vipulnarang95 Yes, it seems add_items_to_collection redirects to this view: https://github.com/django-cms/djangocms-moderation/blob/e2e8ac25bf4f9b42f6a56d2a9fb32df3fd74d94a/djangocms_moderation/views.py#L31. So, it'll be great if we could test if this view reacts if you tried to add, say, an already published object to the moderation request. (Or, maybe, that's already done somewhere else?)

vipulnarang95 commented 22 hours ago

@fsbraun Agreed but this redirects to the moderation view by just checking the versions and there is no logic in the code to check if the versions are draft/published. It just checks if there is version of the content model or not. Also this view is already been tested in test_views.py

fsbraun commented 22 hours ago

I'm not sure if I understand. Are you saying that you can add published versions to a moderation request? Does that make sense? I'd expect to get an error message if I tried to do this.

I now looked at the code and see that the CollectionItemForm does just drop unsutable versions (like published versions) from the selection. (And that's covered by tests.)

vipulnarang95 commented 22 hours ago

I'm not sure if I understand. Are you saying that you can add published versions to a moderation request? Does that make sense? I'd expect to get an error message if I tried to do this. No this gets dropped from the collection as this doesn't makes any sense.. I now looked at the code and see that the CollectionItemForm does just drop unsutable versions (like published versions) from the selection. (And that's covered by tests.) Yes that is covered which I was saying

vipulnarang95 commented 22 hours ago

Also @fsbraun can you also give me the access to release these addons?