IBX-8980: Fixed getGroupedFields method implementation #77

Sztig · 2024-11-21T11:07:49Z

🎫 Issue	IBX-8980

Related PRs:

https://github.com/ibexa/corporate-account/pull/261

Description:

Based on the interface of GroupedContentFormFieldsProvider the method getGroupedFields is implemented incorrectly. The description of the interface states Array of fieldGroupIdentifier grouped by fieldGroupName should be returned. Right now it doesn't return the fieldGroupIdentifier but a translation of said identifier.

For the example the previously retrieved key would be Basic Information, after the fix it's going to be basic_information

sonarqubecloud · 2024-11-21T11:08:15Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ViniTou · 2024-12-09T11:37:31Z

src/lib/Content/Form/Provider/GroupedContentFormFieldsProvider.php

        $groupedFields = [];

        foreach ($fieldsDataForm as $fieldForm) {
            /** @var \Ibexa\Contracts\ContentForms\Data\Content\FieldData $fieldData */
            $fieldData = $fieldForm->getViewData();
            $fieldGroupIdentifier = $this->fieldsGroupsList->getFieldGroup($fieldData->fieldDefinition);
-            $fieldGroupName = $fieldsGroups[$fieldGroupIdentifier] ?? $this->fieldsGroupsList->getDefaultGroup();


Even tho it seems like a bug, not sure we can fix it that way due to bc policy. Otoh, this seems so internal and if only used by that one place (is it tho?) it should be acceptable.
@ibexa/php-dev

mikadamczyk

It seems that $fieldGroupName which is a key in the returned array is used in other places (admin-ui, product-catalog).

https://github.com/ibexa/admin-ui/blob/4.6/src/bundle/Resources/views/themes/admin/content/edit_base.html.twig#L52
https://github.com/ibexa/product-catalog/blob/4.6/src/bundle/Resources/views/themes/admin/product_catalog/product/create.html.twig#L77
Instead of the human-readable name identifier will be displayed

Sztig · 2025-01-14T15:12:20Z

Alright, I will try then implementing a new provider and update the templates in corporate-account to use said provider instead.

… to avoid BC break

Steveb-p · 2025-01-21T10:23:30Z

src/lib/Content/Form/Provider/NonLocalizedGroupedContentFormFieldsProvider.php

+    /** @var \Ibexa\Core\Helper\FieldsGroups\FieldsGroupsList */
+    private $fieldsGroupsList;


Prefer typed properties over annotations, when dealing with PHP 7.4+ and no BC.

Suggested change

/** @var \Ibexa\Core\Helper\FieldsGroups\FieldsGroupsList */

private $fieldsGroupsList;

private FieldsGroupsList $fieldsGroupsList;

Steveb-p · 2025-01-21T10:32:16Z

tests/lib/Content/Form/Provider/GroupedContentFormFieldsProviderTest.php

-            ]);
-
-        $subject = new GroupedContentFormFieldsProvider($fieldsGroupsListMock);
+        $subject = new NonLocalizedGroupedContentFormFieldsProvider($fieldsGroupsListMock);


This - given it's a separate class - should be in it's own test case/class. Currently you're replacing a test for existing class with this one.

👍 I will make a new class for tests for the new provider and revert the old test as they were.

mikadamczyk · 2025-02-03T08:29:59Z

src/lib/Content/Form/Provider/GroupedContentFormFieldsProvider.php

@@ -13,6 +13,9 @@
 use JMS\TranslationBundle\Model\Message;
 use JMS\TranslationBundle\Translation\TranslationContainerInterface;

+/**
+ * @deprecated since 4.6.17 this class is deprecated. Please use NonLocalizedGroupedContentFormFieldsProvider instead


FQCN could be used here

Steveb-p

LGTM in general, and I don't feel like pushing more changes into tests is worth it, so I'm refraining from requesting more changes there :)

Steveb-p · 2025-02-25T10:37:19Z

src/lib/Content/Form/Provider/AbstractGroupedContentFormFieldsProvider.php

+{
+    protected FieldsGroupsList $fieldsGroupsList;
+
+    protected array $groupContext;


This is not used in abstract, and can be moved as private property to GroupedContentFormFieldsProvider.

Steveb-p · 2025-02-25T10:38:24Z

src/lib/Content/Form/Provider/AbstractGroupedContentFormFieldsProvider.php

+        $this->fieldsGroupsList = $fieldsGroupsList;
+    }
+
+    public function getGroupedFields(array $fieldsDataForm): array


We don't expect this method to be overloaded by descending classes, so let's block it off immediately.

Suggested change

public function getGroupedFields(array $fieldsDataForm): array

final public function getGroupedFields(array $fieldsDataForm): array

Steveb-p · 2025-02-25T10:39:45Z

tests/lib/Content/Form/Provider/AbstractGroupedContentFormFieldsProviderTest.php

+    protected function getFieldsGroupsListMock(): FieldsGroupsList
+    {
+        $mock = $this->createMock(FieldsGroupsList::class);
+        $matcher = $this->exactly(3);


We do have team preference to call static methods statically only.

Suggested change

$matcher = $this->exactly(3);

$matcher = self::exactly(3);

Steveb-p · 2025-02-25T10:39:56Z

tests/lib/Content/Form/Provider/AbstractGroupedContentFormFieldsProviderTest.php

+            ->getMock();
+
+        $formMock
+            ->expects($this->once())


Suggested change

->expects($this->once())

->expects(self::once())

Steveb-p · 2025-02-25T10:40:04Z

tests/lib/Content/Form/Provider/AbstractGroupedContentFormFieldsProviderTest.php

+            ]));
+
+        $formMock
+            ->expects($this->once())


Suggested change

->expects($this->once())

->expects(self::once())

Steveb-p · 2025-02-25T10:41:18Z

tests/lib/Content/Form/Provider/AbstractGroupedContentFormFieldsProviderTest.php

+        return $formMock;
+    }
+
+    protected function getTestForms(): array


Nitpick: can be final.

Suggested change

protected function getTestForms(): array

final protected function getTestForms(): array

Steveb-p · 2025-02-25T10:41:36Z

tests/lib/Content/Form/Provider/IdentifiedGroupedContentFormFieldsProviderTest.php

+
+use Ibexa\ContentForms\Content\Form\Provider\IdentifiedGroupedContentFormFieldsProvider;
+
+class IdentifiedGroupedContentFormFieldsProviderTest extends AbstractGroupedContentFormFieldsProviderTest


Suggested change

class IdentifiedGroupedContentFormFieldsProviderTest extends AbstractGroupedContentFormFieldsProviderTest

final class IdentifiedGroupedContentFormFieldsProviderTest extends AbstractGroupedContentFormFieldsProviderTest

Steveb-p · 2025-02-25T10:42:07Z

tests/lib/Content/Form/Provider/AbstractGroupedContentFormFieldsProviderTest.php

+
+abstract class AbstractGroupedContentFormFieldsProviderTest extends TestCase
+{
+    protected function getFieldsGroupsListMock(): FieldsGroupsList


Suggested change

protected function getFieldsGroupsListMock(): FieldsGroupsList

final protected function getFieldsGroupsListMock(): FieldsGroupsList

Steveb-p · 2025-02-25T10:42:15Z

tests/lib/Content/Form/Provider/AbstractGroupedContentFormFieldsProviderTest.php

+        return $mock;
+    }
+
+    protected function getFormMockWithFieldData(


Suggested change

protected function getFormMockWithFieldData(

final protected function getFormMockWithFieldData(

sonarqubecloud · 2025-02-25T10:57:18Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

tomaszszopinski

QA approved on Ibexa DXP 4.6 commerce.

Sztig · 2025-03-06T08:52:45Z

merged up 63d4583

Sztig requested a review from a team November 21, 2024 11:07

Sztig added the Ready for review label Nov 22, 2024

ViniTou reviewed Dec 9, 2024

View reviewed changes

ViniTou requested a review from a team December 9, 2024 11:37

mikadamczyk reviewed Jan 14, 2025

View reviewed changes

github-actions bot assigned mikadamczyk Jan 20, 2025

Sztig removed the Ready for review label Jan 20, 2025

Sztig added 4 commits January 20, 2025 14:39

fix: returning grouped fields with untranslated keys

f52e445

updated testGetGroupedFields test

b3bd5d0

reverted the implementation, marked the provider as deprecated

1f6fdd0

implemented new provider with correct implementation of the interface…

d53ab22

… to avoid BC break

Sztig force-pushed the IBX-8980-grouped-fields-fix branch from 81aef34 to d53ab22 Compare January 20, 2025 13:44

updated tests to use new provider

f7f35b5

Sztig added the Ready for review label Jan 21, 2025

Steveb-p reviewed Jan 21, 2025

View reviewed changes

Sztig added 2 commits January 21, 2025 13:57

implemented suggested change

a6b7d2a

added new test case for new provider and refactored existing one

731f31c

adamwojs requested a review from a team February 2, 2025 11:28

mikadamczyk approved these changes Feb 3, 2025

View reviewed changes

implemented suggest changes

9759292

Sztig requested a review from a team February 4, 2025 15:21

Sztig requested review from Steveb-p and ViniTou February 19, 2025 13:58

Sztig added 4 commits February 21, 2025 17:16

refactored the providers to avoid code duplication

dd6ae49

moved prepareGroupContext to the getGroupKey method

5654a37

improved the getGroupKey method

7723fdf

improved test (avoiding deprecated methods)

a5ed1eb

Steveb-p approved these changes Feb 25, 2025

View reviewed changes

implemented suggested changes

8d8075f

Sztig added Ready for QA and removed Ready for review labels Feb 25, 2025

tomaszszopinski approved these changes Mar 5, 2025

View reviewed changes

tomaszszopinski added QA approved and removed Ready for QA labels Mar 5, 2025

ViniTou approved these changes Mar 5, 2025

View reviewed changes

ViniTou merged commit 3edd454 into 4.6 Mar 5, 2025
15 checks passed

ViniTou deleted the IBX-8980-grouped-fields-fix branch March 5, 2025 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-8980: Fixed getGroupedFields method implementation #77

IBX-8980: Fixed getGroupedFields method implementation #77

Sztig commented Nov 21, 2024

sonarqubecloud bot commented Nov 21, 2024

ViniTou Dec 9, 2024

mikadamczyk left a comment •

edited

Loading

Sztig commented Jan 14, 2025

Steveb-p Jan 21, 2025

Steveb-p Jan 21, 2025

Sztig Jan 21, 2025

mikadamczyk Feb 3, 2025

Steveb-p left a comment

Steveb-p Feb 25, 2025

Steveb-p Feb 25, 2025

Steveb-p Feb 25, 2025

Steveb-p Feb 25, 2025

Steveb-p Feb 25, 2025

Steveb-p Feb 25, 2025

Steveb-p Feb 25, 2025

Steveb-p Feb 25, 2025

Steveb-p Feb 25, 2025

sonarqubecloud bot commented Feb 25, 2025

tomaszszopinski left a comment

Sztig commented Mar 6, 2025

		/** @var \Ibexa\Core\Helper\FieldsGroups\FieldsGroupsList */
		private $fieldsGroupsList;

	/** @var \Ibexa\Core\Helper\FieldsGroups\FieldsGroupsList */
	private $fieldsGroupsList;
	private FieldsGroupsList $fieldsGroupsList;

	public function getGroupedFields(array $fieldsDataForm): array
	final public function getGroupedFields(array $fieldsDataForm): array

	protected function getTestForms(): array
	final protected function getTestForms(): array


		use Ibexa\ContentForms\Content\Form\Provider\IdentifiedGroupedContentFormFieldsProvider;

		class IdentifiedGroupedContentFormFieldsProviderTest extends AbstractGroupedContentFormFieldsProviderTest

	class IdentifiedGroupedContentFormFieldsProviderTest extends AbstractGroupedContentFormFieldsProviderTest
	final class IdentifiedGroupedContentFormFieldsProviderTest extends AbstractGroupedContentFormFieldsProviderTest

	protected function getFieldsGroupsListMock(): FieldsGroupsList
	final protected function getFieldsGroupsListMock(): FieldsGroupsList

	protected function getFormMockWithFieldData(
	final protected function getFormMockWithFieldData(

IBX-8980: Fixed getGroupedFields method implementation #77

IBX-8980: Fixed getGroupedFields method implementation #77

Conversation

Sztig commented Nov 21, 2024

Related PRs:

Description:

sonarqubecloud bot commented Nov 21, 2024

Quality Gate passed

Choose a reason for hiding this comment

mikadamczyk left a comment • edited Loading

Choose a reason for hiding this comment

Sztig commented Jan 14, 2025

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

Steveb-p 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

sonarqubecloud bot commented Feb 25, 2025

Quality Gate passed

tomaszszopinski left a comment

Choose a reason for hiding this comment

Sztig commented Mar 6, 2025

mikadamczyk left a comment •

edited

Loading