ENH Better versioned history #209

emteknetnz · 2024-02-08T05:26:09Z

Issue #10

May require silverstripe/silverstripe-versioned-admin#198 to be merged in order for archive admin to work properly

Note that for elemental page history where you have block with a has_one linkfield on it, the value for this nested field will not display. This is consistent with nested file's not showing on the history of an elemental-file-block with a linked file on it which also does not show on the page history. In both instances, clicking on the "Block history" link will show the value of nested field

emteknetnz · 2024-02-12T06:11:15Z

client/src/components/LinkField/LinkField.js

@@ -5,7 +5,6 @@ import { connect } from 'react-redux';
 import { DndContext, closestCenter, PointerSensor, useSensor, useSensors } from '@dnd-kit/core';
 import { arrayMove, SortableContext, verticalListSortingStrategy } from '@dnd-kit/sortable';
 import { restrictToVerticalAxis, restrictToParentElement } from '@dnd-kit/modifiers';
-import { injectGraphql } from 'lib/Injector';


The is unrelated refactoring to remove an unused import

emteknetnz · 2024-02-12T06:11:51Z

client/src/components/LinkField/LinkField.js

 */
 const LinkField = ({
  value = null,
-  onChange,
-  onNonPublishedVersionedState,


The is unrelated refactoring to remove a non-existent prop

emteknetnz · 2024-02-12T06:12:52Z

client/src/components/LinkField/LinkField.js

          setLoading(false);
          // isSorting is set to true on drag start and only set to false here to prevent
          // the loading indicator for flickering
          setIsSorting(false);
        })
-        .catch(() => {
-          actions.toasts.error(i18n._t('LinkField.FAILED_TO_LOAD_LINKS', 'Failed to load links'))


Need to not show a red toast here because this will pop up when viewing old versions of an owner where the link has been deleted. I've changed this to show an inline message when the link fails to load.

emteknetnz · 2024-02-12T06:13:32Z

client/src/components/LinkField/LinkField.js

@@ -350,7 +362,7 @@ const LinkField = ({

 LinkField.propTypes = {
  value: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.number), PropTypes.number]),
-  onChange: PropTypes.func.isRequired,
+  onChange: PropTypes.func,


This should never have been a required field as it only makes sense in an entwine context

emteknetnz · 2024-02-12T06:15:03Z

client/src/containers/LinkModalContainer.js

@@ -23,6 +23,7 @@ const LinkModalContainer = ({ types, typeKey, linkID = 0, isOpen, onSuccess, onC
  // which will causes bugs with validation
  if (!LinkModal) {
    setLinkModal(() => loadComponent(`LinkModal.${handlerName}`));
+    return;


This should is to fix a console error where the state hasn't yet been updated from the call the setLinkModal, meaning that the <LinkModal> jsx object doesn't yet exist at time of render because LinkModal hasn't yet been set. Setting the state via this hook will trigger a rerender in which time <LinkModal> will not exist

emteknetnz · 2024-02-12T06:19:38Z

client/lang/en.js

@@ -21,7 +21,7 @@ if (typeof(ss) === 'undefined' || typeof(ss.i18n) === 'undefined') {
    "LinkField.LINK_MODIFIED_TITLE": "Link has unpublished changes",
    "LinkField.LINK_MODIFIED_LABEL": "Modified",
    "LinkField.CANNOT_CREATE_LINK": "Cannot create link",
-    "LinkField.FAILED_TO_LOAD_LINKS": "Failed to load links",
+    "LinkField.FAILED_TO_LOAD_LINKS": "Failed to load link(s)",


This is a minor improvement to the message as it's used for both LinkField and for MultiLinkField

emteknetnz · 2024-02-12T06:20:33Z

client/src/components/LinkField/LinkField.js

 */
 const LinkField = ({
  value = null,
-  onChange,
-  onNonPublishedVersionedState,
+  onChange = () => {},


This is a default value that's being set now that it's no longer a required prop

emteknetnz · 2024-02-12T06:23:52Z

client/src/components/LinkField/tests/LinkField-test.js

@@ -66,16 +76,21 @@ test('LinkField will render loading indicator if ownerID is not 0', async () =>
  expect(container.querySelectorAll('.link-picker')).toHaveLength(1);
 });

-test('LinkField will render link-picker if ownerID is not 0 and has finished loading', async () => {
+test('LinkField will render link-picker if ownerID is not 0 and isMulti and has finished loading', async () => {


This jest test was technically wrong before - it shouldn't render a .link-picker element if it's non-isMulti and ownerID is not 0 - it should only render that if it's isMulti

GuySartorelli

Still need to fulfil this acceptance critera (or deem it too hard for this card and open a separate card for them):

Archived Admin uses the same behaviour as Versionned Admin when displaying link.

If I click on an archived page from archive admin, I see:

Note that there was a link in both the "Has one link" and "Has many links one" field, but not the "Has many links two" field.

LinkField on versioned history view correctly identify if a Link was attached to the page, but the Link was deleted.

This doesn't seem to work.

Create a link on a page
Publish the page
Archive the link
Make a change to the page and publish it again
View the version of the page which did have the link

For me it shows "Failed to load link(s)"

Issue with archived history

When I archive a page (which has links on it), then restore the page from archived, then in the history tab I try to view the archive version, I get an infinite loading component.

emteknetnz · 2024-02-13T03:22:22Z

Archived Admin uses the same behaviour as Versionned Admin when displaying link.

ArchivedAdmin support adds a level of complexity over the regular history viewer that I don't think is worth it. We need to support multiple permutations of owner and links archived and not-archived. The LinkFieldController which handles all the data isn't aware of this. We also need to add support for knowing if we're even in an archived admin which is very messy because of the special treatment given to SiteTree vs other dataobjects.

LinkField on versioned history view correctly identify if a Link was attached to the page, but the Link was deleted.

This relates to the point above. Because the link was deleted then when attempting to fetch the link via LinkFieldController it will return a 404. We could change the message from "Failed to load link(s)" to "Link was deleted" ... but then again that's an assumption because it may be 404'ing for a different reason. I'd say it's such a minor point we should just leave as is.

Issue with archived history

I was unable to replicate this issue

GuySartorelli

LGTM, definitely an improvement over how it was before. Just a bit of tidy up.

src/Form/LinkField.php

src/Form/MultiLinkField.php

GuySartorelli

LGTM

emteknetnz mentioned this pull request Feb 8, 2024

Link versioned history #10

Closed

emteknetnz force-pushed the pulls/4/version-history branch 4 times, most recently from b7eab31 to acbbed5 Compare February 12, 2024 06:07

emteknetnz marked this pull request as ready for review February 12, 2024 06:09

emteknetnz commented Feb 12, 2024

View reviewed changes

emteknetnz force-pushed the pulls/4/version-history branch 3 times, most recently from 1d6eb0b to 2d01f5b Compare February 12, 2024 06:31

GuySartorelli requested changes Feb 12, 2024

View reviewed changes

emteknetnz force-pushed the pulls/4/version-history branch from 2d01f5b to be97b78 Compare February 13, 2024 02:47

emteknetnz force-pushed the pulls/4/version-history branch from be97b78 to 6e25096 Compare February 13, 2024 04:32

GuySartorelli requested changes Feb 13, 2024

View reviewed changes

src/Form/LinkField.php Outdated Show resolved Hide resolved

src/Form/MultiLinkField.php Outdated Show resolved Hide resolved

emteknetnz force-pushed the pulls/4/version-history branch 2 times, most recently from eace65b to 79e6307 Compare February 13, 2024 23:57

GuySartorelli reviewed Feb 14, 2024

View reviewed changes

src/Form/MultiLinkField.php Outdated Show resolved Hide resolved

emteknetnz force-pushed the pulls/4/version-history branch from 79e6307 to e3871a4 Compare February 14, 2024 01:15

ENH Better versioned history

3e97f0d

emteknetnz force-pushed the pulls/4/version-history branch from e3871a4 to 3e97f0d Compare February 14, 2024 02:08

GuySartorelli approved these changes Feb 14, 2024

View reviewed changes

GuySartorelli merged commit 58bd7a7 into silverstripe:4 Feb 14, 2024
12 checks passed

GuySartorelli deleted the pulls/4/version-history branch February 14, 2024 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Better versioned history #209

ENH Better versioned history #209

emteknetnz commented Feb 8, 2024 •

edited

Loading

emteknetnz Feb 12, 2024

emteknetnz Feb 12, 2024

emteknetnz Feb 12, 2024

emteknetnz Feb 12, 2024

emteknetnz Feb 12, 2024 •

edited

Loading

emteknetnz Feb 12, 2024

emteknetnz Feb 12, 2024

emteknetnz Feb 12, 2024

GuySartorelli left a comment •

edited

Loading

emteknetnz commented Feb 13, 2024

GuySartorelli left a comment

GuySartorelli left a comment

ENH Better versioned history #209

ENH Better versioned history #209

Conversation

emteknetnz commented Feb 8, 2024 • edited Loading

emteknetnz Feb 12, 2024

Choose a reason for hiding this comment

emteknetnz Feb 12, 2024

Choose a reason for hiding this comment

emteknetnz Feb 12, 2024

Choose a reason for hiding this comment

emteknetnz Feb 12, 2024

Choose a reason for hiding this comment

emteknetnz Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

emteknetnz Feb 12, 2024

Choose a reason for hiding this comment

emteknetnz Feb 12, 2024

Choose a reason for hiding this comment

emteknetnz Feb 12, 2024

Choose a reason for hiding this comment

GuySartorelli left a comment • edited Loading

Choose a reason for hiding this comment

Archived Admin uses the same behaviour as Versionned Admin when displaying link.

LinkField on versioned history view correctly identify if a Link was attached to the page, but the Link was deleted.

Issue with archived history

emteknetnz commented Feb 13, 2024

GuySartorelli left a comment

Choose a reason for hiding this comment

GuySartorelli left a comment

Choose a reason for hiding this comment

emteknetnz commented Feb 8, 2024 •

edited

Loading

emteknetnz Feb 12, 2024 •

edited

Loading

GuySartorelli left a comment •

edited

Loading