silverstripe
Make 'next review date' dynamic
I'm evaluating this module for a client, looks promising but noted a few issues and want to suggest a possible fix.
Issues:
- On installing the module and setting a default review date in Settings, this is not applied retroactively to existing pages automatically. You would need to save each page individually to trigger the addition of a NextReviewDate.
- Marking a page as reviewed causes the NextReviewDate to be updated, requiring a write on the page which changes the page status to change to Draft (#22)
- If you change your mind about how often pages should be reviewed, this won't be applied to a page until the next time you review it, because the NextReviewDate has already been locked in.
The solution I would propose for all of this is to remove the 'NextReviewDate' database field, and instead infer this information based on the last time it was reviewed. This information is already stored separately to the page in ContentReviewLogs, which could be interpreted by a getNextReviewDate() method.
The only thing I'm not sure about is performance impact when getting a list of pages that are due for review. Since the review period would have to be evaluated for each page I think it would be necessary to loop over every page and run the getNextReviewDate() method, instead of filtering the DataList on the NextReviewDate field. I wouldn't expect this to be a big problem though.
Does this sound like a good idea?
I've started work on this at https://github.com/jonom/silverstripe-contentreview/commit/501d6121e5c883b758bc7005ff65856bfa17560c seems to be working well! If you guys are interested in adopting these changes let me know. It would be a breaking change, and tests would need to re-written.
There is a negative performance impact from these changes but I think that is outweighed by the benefits.
Hey @jonom, FYI if you're interested in continuing this work, we have a couple of months until the next stable major release goes out so a good time to break stuff
@robbieaverill do you agree with the changes in general? The functionality is implemented in https://github.com/jonom/silverstripe-contentreview/commit/501d6121e5c883b758bc7005ff65856bfa17560c if you want to try it out. Would need to add docs and tests but want to make sure the changes are mergeable before going to the effort.
@jonom I've only skim read your points, but personally I think it sounds sensible to store this data outside of the SiteTree record itself so it doesn't need to be coupled with versioning.
If you do decide to update this PR for SS4 be aware that CLDR date formats via DBDatetime are now the standard, but there's still some occurrences of using straight up date() calls to format dates, which use PHP date formats.
Interesting idea but it's an old issue that we're not going to do anything with any time soon. Closing. If someone is keen for this and wants to put in the work to implement it, @ me and we can revisit.
