ENH UI updated for versioned objects and all its relations #11101

sabina-talipova · 2023-12-20T19:58:18Z

Description

These changes add additional checking for the presence of changes in child classes (which have the Versioned extension) and if these changes exist, then the “MODIFIED” badge will be added to the name of the object. In my opinion, this is the only suitable badge, since there are several points that make the use of other badges difficult. For example, if the object itself has several child objects and these objects have different states, for example, one has the “DRAFT” state, and the other has the “ARCHIVED” state.

Parent issue

SPIKE Update UI to account for a more nuance understanding of unpublished silverstripe-admin#1593

sabina-talipova · 2023-12-21T20:03:36Z

src/Forms/GridField/GridFieldVersionTag.php

+     */
+    private function getStatusFlags(DataObject $record): array
+    {
+        if ($record->hasExtension(Versioned::class)) {


This check that this class does not have the Versioned extension is needed in order, firstly, not to have a duplicate badge and, secondly, not to override the badge of the current state of the current object.

So there is already a versioned badge, and this PR is adding a new one? Is the original status badge wrong? Where is the code for the original status badge. Would it make sense to move this code there, or at least deprecate the old code since it's presumably wrong?

sabina-talipova · 2023-12-21T20:11:00Z

src/Forms/GridField/GridFieldDetailForm_ItemRequest.php

@@ -417,7 +420,7 @@ protected function getFormActions()
                    throw new LogicException(get_class($this->record) . ' must implement ' . DataObjectInterface::class);
                }

-                $noChangesClasses = 'btn-outline-primary font-icon-tick';
+                $noChangesClasses = $this->stagesDifferRecursive() ? 'btn-primary font-icon-save' : 'btn-outline-primary font-icon-tick';


This check is needed to highlight “Save” if child versioned objects have changes

GuySartorelli

Not a proper review, just something I noticed at a glance.

GuySartorelli · 2024-01-07T21:37:09Z

src/Forms/Form.php

@@ -1483,7 +1483,7 @@ public function loadDataFrom($data, $mergeStrategy = 0, $fieldList = null)
     * @param FieldList $fieldList An optional list of fields to process.  This can be useful when you have a
     * form that has some fields that save to one object, and some that save to another.
     */
-    public function saveInto(DataObjectInterface $dataObject, $fieldList = null)
+    public function saveInto(ViewableData|DataObjectInterface $dataObject, $fieldList = null)


We can't change the method signature of public API

Suggested change

public function saveInto(ViewableData|DataObjectInterface $dataObject, $fieldList = null)

public function saveInto(DataObjectInterface $dataObject, $fieldList = null)

emteknetnz · 2024-01-07T22:34:45Z

tests/php/Forms/GridField/GridFieldVersionTagTest.yml

+SilverStripe\Forms\Tests\GridField\GridFieldTest\Cheerleader:
+  cheerleader1_team1:
+    Name: Heather
+    Team: =>SilverStripe\Forms\Tests\GridField\GridFieldTest\Team.team1


Add a blank line at end of file

emteknetnz · 2024-01-07T22:35:05Z

tests/php/Forms/GridField/GridFieldVersionTagTest.yml

@@ -0,0 +1,8 @@
+SilverStripe\Forms\Tests\GridField\GridFieldTest\Team:
+    team1:


Indentation in this file is wrong, should be 2 spaces

emteknetnz · 2024-01-07T22:35:38Z

tests/php/Forms/GridField/GridFieldDetailForm_ItemRequestTest.yml

@@ -0,0 +1,8 @@
+SilverStripe\Forms\Tests\GridField\GridFieldTest\Team:
+    team1:


Indentation in this file is wrong, should be 2 spaces

Also add a blank line at end of the file

emteknetnz · 2024-01-07T22:37:36Z

src/Forms/GridField/GridFieldVersionTag.php

+     */
+    private function getStatusFlags(DataObject $record): array
+    {
+        if ($record->hasExtension(Versioned::class)) {


So there is already a versioned badge, and this PR is adding a new one? Is the original status badge wrong? Where is the code for the original status badge. Would it make sense to move this code there, or at least deprecate the old code since it's presumably wrong?

emteknetnz · 2024-01-07T22:39:00Z

src/Forms/GridField/GridFieldVersionTag.php

+     * @param string $columnName
+     * @return string HTML for the column. Return NULL to skip.
+     */
+    public function getColumnContent($gridField, $record, $columnName)


Strongly type params and return type, remove docblocks for param types unless there is useful information for a param/return type. Do this for all methods in the PR

There is a small problem, because interface GridField_ColumnProvider still doesn't support strong types, so I can update only few methods.

sabina-talipova · 2024-01-08T01:54:09Z

So there is already a versioned badge, and this PR is adding a new one?

We need this check in case when parent DataObject has Versioned extension and has current status DRAFT or any other status than in child / grandchild objects, since this badge will be provided in Versioned extension. In another cases, I assume we should add this badge only if parent object doesn't have Versioned extension, but child / grandchild objects does.

Is the original status badge wrong?

We have the original status badge only if parent object has Versioned extension.

Where is the code for the original status badge. Would it make sense to move this code there, or at least deprecate the old code since it's presumably wrong?

All status badges are part of Versioned extension. So the main problem that require these changes it's if the parent object in the relations doesn't have Versioned extension

michalkleiner · 2024-01-08T02:16:50Z

Wouldn't it be confusing if the parent object doesn't have Versioned extension and would still show Modified? Perhaps we need another label when it's just the children?

sabina-talipova force-pushed the pulls/5/versioned-ui branch 3 times, most recently from ce8d69b to f79ecfa Compare December 21, 2023 19:37

sabina-talipova marked this pull request as ready for review December 21, 2023 20:00

sabina-talipova commented Dec 21, 2023

View reviewed changes

sabina-talipova mentioned this pull request Dec 21, 2023

SPIKE Update UI to account for a more nuance understanding of unpublished silverstripe/silverstripe-admin#1593

Open

sabina-talipova mentioned this pull request Jan 7, 2024

ENH Updated UI for versioned objects and all its versioned relations silverstripe/silverstripe-cms#2912

Open

GuySartorelli requested changes Jan 7, 2024

View reviewed changes

emteknetnz requested changes Jan 7, 2024

View reviewed changes

sabina-talipova force-pushed the pulls/5/versioned-ui branch from f79ecfa to 107d9d9 Compare January 8, 2024 02:42

ENH UI updated for versioned objects and all its relations

c188ac7

sabina-talipova force-pushed the pulls/5/versioned-ui branch from 107d9d9 to c188ac7 Compare January 8, 2024 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH UI updated for versioned objects and all its relations #11101

ENH UI updated for versioned objects and all its relations #11101

sabina-talipova commented Dec 20, 2023 •

edited

sabina-talipova Dec 21, 2023

emteknetnz Jan 7, 2024

sabina-talipova Dec 21, 2023

GuySartorelli left a comment

GuySartorelli Jan 7, 2024

sabina-talipova Jan 8, 2024

emteknetnz Jan 7, 2024

sabina-talipova Jan 8, 2024

emteknetnz Jan 7, 2024

sabina-talipova Jan 8, 2024

emteknetnz Jan 7, 2024

sabina-talipova Jan 8, 2024

emteknetnz Jan 7, 2024

emteknetnz Jan 7, 2024

sabina-talipova Jan 8, 2024

sabina-talipova commented Jan 8, 2024

michalkleiner commented Jan 8, 2024

	public function saveInto(ViewableData\|DataObjectInterface $dataObject, $fieldList = null)
	public function saveInto(DataObjectInterface $dataObject, $fieldList = null)

		@@ -0,0 +1,8 @@
		SilverStripe\Forms\Tests\GridField\GridFieldTest\Team:
		team1:

ENH UI updated for versioned objects and all its relations #11101

Are you sure you want to change the base?

ENH UI updated for versioned objects and all its relations #11101

Conversation

sabina-talipova commented Dec 20, 2023 • edited

Description

Parent issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabina-talipova commented Jan 8, 2024

michalkleiner commented Jan 8, 2024

sabina-talipova commented Dec 20, 2023 •

edited