Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more CRUD form tests #1552

Merged
merged 8 commits into from
Aug 12, 2022
Merged

Add more CRUD form tests #1552

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8264f9d
Add more CRUD form tests
jordisala1991 Aug 11, 2022
d40cf30
Fix tests
jordisala1991 Aug 11, 2022
9a48a2f
Update composer.json
jordisala1991 Aug 11, 2022
fcc9daa
More block tests
jordisala1991 Aug 12, 2022
28f87f0
Remove old code
jordisala1991 Aug 12, 2022
eb43569
Remove PageFormMapper
jordisala1991 Aug 12, 2022
d922ffe
Simplify logic
jordisala1991 Aug 12, 2022
bdde78e
Simplify more
jordisala1991 Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Block/SharedBlockBlockService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,12 @@ private function getBlockBuilder(): FormBuilderInterface
'translation_domain' => 'SonataPageBundle',
'edit' => 'list',
]);
$fieldDescription->setAssociationAdmin($this->sharedBlockAdmin);

return $this->sharedBlockAdmin->getFormBuilder()->create('blockId', ModelListType::class, [
return $this->sharedBlockAdmin->getFormContractor()->getFormBuilder(
$this->sharedBlockAdmin->getUniqId(),
['data_class' => $this->sharedBlockAdmin->getClass()]
)->create('blockId', ModelListType::class, [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of fix should be applied to other bundles. There is still a lot of code to write to do such fields.

There were 2 problems:

  1. Field description didn't had association admin, and it break on a template from sonataAdmin, fixed line 157.
  2. getFormBuilder does a lot of things, it instantiate al the form for that admin, and it needs a subject, so it breaks. I picked the first lines where it creates de form builder. There are other options to fix this too. One of them is use the original $form from above, but we are wrapping it on a PageFormMapper (For no reason).

IMO we might refactor this further, and remove PageFormMapper too, it is not needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the issue ? And in which bundle should we apply the same fix ?

It was never reported before

I would say that PageFormMapper was maybe needed prior to sonata-project/SonataBlockBundle@cd2f8f3. But now the AdminFormMapper implements the BlockBundle FormMapper it's not needed.

Does removing this class (in this PR) would simplify your bugfix ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is:

  1. Some of our bundles define blocks (and they do require block bundle) 👍
  2. Some of that blocks are EditableBlocks (an interface defined on block bundle) 👍
  3. That interface is only testable HERE, because those editableBlocks are for SonataPageBundle 👎
  4. When we move for example SonataMediaBundle from 3.x to 4.x we migrated those blocks to newer SonataBlock version, but we didn't test those because of point 3. (and we didn't try to unit test AFAIK, will need to check) 👎
  5. Now that we are migrating this bundle, some errors appear.

Those are other examples of the same problem:
https://github.com/sonata-project/SonataMediaBundle/blob/4.x/src/Block/MediaBlockService.php#L210-L226

I don't know if there are a lot of them (I think not), but this kind of dependencies are hard to test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at my last commit, not using PageFormMapper, allows to \assert for AdminFormMapper and we have getFormBuilder there.

'sonata_field_description' => $fieldDescription,
'class' => $this->sharedBlockAdmin->getClass(),
'model_manager' => $this->sharedBlockAdmin->getModelManager(),
Expand Down
45 changes: 44 additions & 1 deletion tests/Functional/Admin/SharedBlockAdminTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,54 @@ public function testCrudUrls(string $url, array $parameters = []): void
public static function provideCrudUrlsCases(): iterable
{
yield 'List Block' => ['/admin/tests/app/sonatapageblock/shared/list'];
yield 'Create Block' => ['/admin/tests/app/sonatapageblock/shared/create'];
yield 'List Block types' => ['/admin/tests/app/sonatapageblock/shared/create'];

yield 'Create Block' => ['/admin/tests/app/sonatapageblock/shared/create', [
'type' => 'sonata.page.block.shared_block',
]];

yield 'Edit Block' => ['/admin/tests/app/sonatapageblock/shared/1/edit'];
yield 'Remove Block' => ['/admin/tests/app/sonatapageblock/shared/1/delete'];
}

/**
* @dataProvider provideFormUrlsCases
*
* @param array<string, mixed> $parameters
* @param array<string, mixed> $fieldValues
*/
public function testFormsUrls(string $url, array $parameters, string $button, array $fieldValues = []): void
{
$client = self::createClient();

$this->prepareData();

$client->request('GET', $url, $parameters);
$client->submitForm($button, $fieldValues);
$client->followRedirect();

self::assertResponseIsSuccessful();
}

/**
* @return iterable<array<string|array<string, mixed>>>
*
* @phpstan-return iterable<array{0: string, 1: array<string, mixed>, 2: string, 3?: array<string, mixed>}>
*/
public static function provideFormUrlsCases(): iterable
{
yield 'Create Shared Block - Shared Block' => ['/admin/tests/app/sonatapageblock/shared/create', [
'uniqid' => 'shared_block',
'type' => 'sonata.page.block.shared_block',
], 'btn_create_and_list', [
'shared_block[name]' => 'Name',
'shared_block[settings][blockId]' => 1,
]];

yield 'Edit Shared Block' => ['/admin/tests/app/sonatapageblock/shared/1/edit', [], 'btn_update_and_list', []];
yield 'Remove Shared Block' => ['/admin/tests/app/sonatapageblock/shared/1/delete', [], 'btn_delete'];
}

/**
* @return class-string<KernelInterface>
*/
Expand Down
37 changes: 37 additions & 0 deletions tests/Functional/Admin/SiteAdminTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,43 @@ public static function provideCrudUrlsCases(): iterable
yield 'Snaphosts Site' => ['/admin/tests/app/sonatapagesite/1/snapshots'];
}

/**
* @dataProvider provideFormUrlsCases
*
* @param array<string, mixed> $parameters
* @param array<string, mixed> $fieldValues
*/
public function testFormsUrls(string $url, array $parameters, string $button, array $fieldValues = []): void
{
$client = self::createClient();

$this->prepareData();

$client->request('GET', $url, $parameters);
$client->submitForm($button, $fieldValues);
$client->followRedirect();

self::assertResponseIsSuccessful();
}

/**
* @return iterable<array<string|array<string, mixed>>>
*
* @phpstan-return iterable<array{0: string, 1: array<string, mixed>, 2: string, 3?: array<string, mixed>}>
*/
public static function provideFormUrlsCases(): iterable
{
yield 'Create Site' => ['/admin/tests/app/sonatapagesite/create', [
'uniqid' => 'site',
], 'btn_create_and_list', [
'site[name]' => 'Name',
'site[host]' => 'localhost',
]];

yield 'Edit Site' => ['/admin/tests/app/sonatapagesite/1/edit', [], 'btn_update_and_list', []];
yield 'Remove Site' => ['/admin/tests/app/sonatapagesite/1/delete', [], 'btn_delete'];
}

/**
* @return class-string<KernelInterface>
*/
Expand Down