django-cms / djangocms-versioning

General purpose versioning package for Django CMS 4 and above.
Other
34 stars 30 forks source link

Provide additional information when sending publish/unpublish events #348

Closed GaretJax closed 1 year ago

GaretJax commented 1 year ago

Description

Checklist

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/django-cms/djangocms-versioning/pull/348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=django-cms) | Coverage Δ | | |---|---|---| | [djangocms\_versioning/models.py](https://app.codecov.io/gh/django-cms/djangocms-versioning/pull/348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=django-cms#diff-ZGphbmdvY21zX3ZlcnNpb25pbmcvbW9kZWxzLnB5) | `94.44% <100.00%> (ø)` | |

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

fsbraun commented 1 year ago

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

kinkerl commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

Aiky30 commented 1 year ago

@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 commented 1 year ago

hey @Aiky30 , hope you are good ;)

Added a test which covers the scenario.

fsbraun commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@fsbraun any update? Thanks!

marksweb commented 1 year ago

@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 commented 1 year ago

@kinkerl Thanks for this contribution and the docs update!