Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide additional information when sending publish/unpublish events #348

Merged
merged 11 commits into from
Nov 8, 2023

Conversation

GaretJax
Copy link
Contributor

@GaretJax GaretJax commented Sep 5, 2023

Description

  • When sending post-publishing events, include the list of unpublished versions.
  • When sending post-unpublishing events, include the to-be-published version.

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.

When sending post publishing events, include the list of unpublished
versions.

When sending post unpublishing events, include the version to be
published.
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dda7f5d) 90.86% compared to head (bd8dbb5) 90.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #348   +/-   ##
=======================================
  Coverage   90.86%   90.86%           
=======================================
  Files          72       72           
  Lines        2530     2530           
  Branches      356      356           
=======================================
  Hits         2299     2299           
  Misses        170      170           
  Partials       61       61           
Files Coverage Δ
djangocms_versioning/models.py 94.44% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fsbraun
Copy link
Member

fsbraun commented Sep 12, 2023

@GaretJax Can you provide some information on the use case?

@kinkerl
Copy link
Contributor

kinkerl commented Sep 20, 2023

This allows a developer to hook up the the signal and differentiate between two usecases which currently look similar:

  1. unpublishing of a version
  2. publishing of a newer item which will implicitly unpublish the previous version.

Up until now, both "unpublish" (and publish) signals look the same. With this change, we can see if another version will be published with this unpublish event and handle the signal differently.

@fsbraun
Copy link
Member

fsbraun commented Sep 20, 2023

@kinkerl Thanks for explaining the pull request. I think I understand what you want to achieve. Still, I am not sure why you would need this.

The basic design of djangocms-versioning is based on a finite state machine, i.e. how a version reached its state must be irrelevant, only the current state counts.

For your example, this means: It must not make a difference if I unpublish version x, go to lunch and subsequently publish draft y or if I publish draft y and thereby implicitly unpublish version x within the same second. In both scenarios, the states in the beginning and in the end are the same. I am uncertain if it is a good idea to send different signals for the same process.

In any case, the receiver can easily find out if there is a most recently unpublished version and take some action based on that. This would lead to the same behaviour with or without “lunch” in between.

What use case would need to differentiate between those two scenarios?

@kinkerl
Copy link
Contributor

kinkerl commented Sep 20, 2023

Lets assume we have a page published with versioning on a specific URL and i use elastic search as a search solution.

  1. I am unpublishing a page. In this case, I want to delete the entry for the page in elastic search from the index
  2. I am publishing a newer page (and implicitly unpublish the old). In this case, i want to just update the index without running into race conditions of two conflicting signals.

In any case, the receiver can easily find out if there is a most recently unpublished version and take some action based on that.

I aggree that a receiver can make an assumption/approximation about this which is not certain. However, it gets tricky for a receiver of an unpublish signal to figure out if another page might be going to get published in the future.

@fsbraun
Copy link
Member

fsbraun commented Sep 20, 2023

Thanks, @kinkerl! I'll take this to the tech committee.

@Aiky30
Copy link
Contributor

Aiky30 commented Sep 21, 2023

@kinkerl Hey man, hope you're good, long time.

Are you able to replicate this scenario in a unit test?

This package is by far the most complicated and has the best coverage of any of the V4 packages.

Some example sof signal tests that may help: https://github.com/django-cms/djangocms-versioning/blob/master/tests/test_signals.py

@kinkerl
Copy link
Contributor

kinkerl commented Sep 25, 2023

hey @Aiky30 , hope you are good ;)

Added a test which covers the scenario.

@fsbraun
Copy link
Member

fsbraun commented Sep 25, 2023

Can I also ask for an update of the docs (signals.rst)? Can you please advise that the result of the signal functions called only depends on the final state of the version? Maybe you can give the indexing example (remove from index, add from index can be abbreviated to update index to avoid race conditions.

@kinkerl
Copy link
Contributor

kinkerl commented Sep 26, 2023

No problem. However, i am not quite sure what you mean with "Can you please advise that the result of the signal functions called only depends on the final state of the version?"
I might be missing a bit of information why this is important as we have not changed this behavior.

@kinkerl
Copy link
Contributor

kinkerl commented Oct 20, 2023

@fsbraun any update? Thanks!

@marksweb
Copy link
Member

@kinkerl I think some docs with examples of usage would be great here. The elasticsearch scenario is an ideal one.

I think the integration docs would be most suitable.

@fsbraun
Copy link
Member

fsbraun commented Nov 8, 2023

@kinkerl Thanks for this contribution and the docs update!

@fsbraun fsbraun merged commit 97f2801 into django-cms:master Nov 8, 2023
44 checks passed
joshyu pushed a commit to joshyu/djangocms-versioning that referenced this pull request Apr 2, 2024
fsbraun pushed a commit that referenced this pull request Apr 3, 2024
* feat: Provide additional information when sending publish/unpublish events (cherry-pick #348)

* fix:  Add keyword arguments in VersionAdminMixin render_change_form (cherry-pick #356)

---------

Co-authored-by: Josh Yu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants