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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IBX-3856: Changed Checkbox's value to never be considered empty #336

Merged
merged 5 commits into from
Oct 6, 2022
Merged

IBX-3856: Changed Checkbox's value to never be considered empty #336

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab459ce
Added test
mnocon Sep 22, 2022
e01db7a
IBX-3856: Made value of Checkbox fieldtype never empty
mnocon Sep 22, 2022
9f21465
Removed redundant null check
mnocon Sep 22, 2022
7e8a12c
Fixed integration tests
mnocon Sep 22, 2022
64c5047
Review fixes
mnocon Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 21 additions & 11 deletions eZ/Publish/API/Repository/Tests/FieldType/CheckboxIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,22 +306,17 @@ public function provideFromHashData()

public function providerForTestIsEmptyValue()
{
return [
[new CheckboxValue()],
[new CheckboxValue(null)],
[new CheckboxValue(false)],
];
return [];
}

public function providerForTestIsNotEmptyValue()
{
return [
[
$this->getValidCreationFieldData(),
],
[
new CheckboxValue(true),
],
[$this->getValidCreationFieldData()],
[new CheckboxValue(true)],
[new CheckboxValue()],
[new CheckboxValue(null)],
[new CheckboxValue(false)],
];
}

Expand Down Expand Up @@ -392,6 +387,21 @@ public function testFindContentFieldCriterion(bool $isActive): void
self::assertSame($isActive, $value->bool);
}

public function testAddFieldDefinition()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function testAddFieldDefinition()
public function testAddFieldDefinition(): void

{
$content = $this->addFieldDefinition();

$this->assertCount(2, $content->getFields());
Copy link
Member

Choose a reason for hiding this comment

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

Would like to see sanity check prior addFieldDefinition - assert count = 1

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how to do this - this test is a variation of the base one:

ezplatform-kernel/eZ/Publish/API/Repository/Tests/FieldType/BaseIntegrationTest.php

Lines 1009 to 1022 in 59ca143

public function testAddFieldDefinition()
{
$content = $this->addFieldDefinition();
$this->assertCount(2, $content->getFields());
$this->assertTrue(
$this->getRepository()->getFieldTypeService()->getFieldType(
$this->getTypeName()
)->isEmptyValue(
$content->getFieldValue('data')
)
);
}

But I had to change the assertion.

The method 'addFieldDefinition` first removes the fieldDefinition:

ezplatform-kernel/eZ/Publish/API/Repository/Tests/FieldType/BaseIntegrationTest.php

Line 985 in 59ca143

$content = $this->removeFieldDefinition();

and then adds it again

The flow inside addFieldDefinition looks like this then:

  • The Content item is created (with 2 fields)
  • the tested field definition is removed
  • the tested field definition is added again

Current approach makes the $content variable available only after the call to addFieldDefinition finishes, I can't really add that sanity check without reworking this test.

I wanted to reuse the existing setup methods, but if you think it's good to add some custom setup for this test then I will do it

Copy link
Member

Choose a reason for hiding this comment

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

Static methods should be called statically.

Suggested change
$this->assertCount(2, $content->getFields());
self::assertCount(2, $content->getFields());


$this->assertFalse(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->assertFalse(
self::assertFalse(

$this->getRepository()->getFieldTypeService()->getFieldType(
Copy link
Member

Choose a reason for hiding this comment

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

Readability, please extract

$fieldTypeService = $this->getRepository()->getFieldTypeService()

as the first call of the method. If you need to call it after addFieldDefinition then it should be called as getRepository(false) because the method has side effects.

$this->getTypeName()
)->isEmptyValue(
$content->getFieldValue('data')
)
);
}

protected function createCheckboxContentItems(Repository $repository): void
{
$contentType = $this->createContentTypeWithCheckboxField($repository);
Expand Down
5 changes: 5 additions & 0 deletions eZ/Publish/Core/FieldType/Checkbox/Type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ public function getEmptyValue()
return new Value(false);
}

public function isEmptyValue(SPIValue $value): bool
{
return false;
}

/**
* Inspects given $inputValue and potentially converts it into a dedicated value object.
*
Expand Down
2 changes: 1 addition & 1 deletion eZ/Publish/Core/FieldType/FieldType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ public function onlyEmptyInstance()
*/
public function isEmptyValue(SPIValue $value)
{
return $value === null || $value == $this->getEmptyValue();
return $value == $this->getEmptyValue();
}

/**
Expand Down
26 changes: 26 additions & 0 deletions eZ/Publish/Core/FieldType/Tests/CheckboxTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,4 +297,30 @@ public function provideDataForGetName(): array
[new CheckboxValue(false), '0', [], 'en_GB'],
];
}

/**
* @dataProvider provideForValueIsNeverEmpty
*/
public function testValueIsNeverEmpty(CheckboxValue $value)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function testValueIsNeverEmpty(CheckboxValue $value)
public function testValueIsNeverEmpty(CheckboxValue $value): void

{
$fieldType = $this->getFieldTypeUnderTest();

self::assertFalse($fieldType->isEmptyValue($value));
}

/**
* @return iterable<array{
* \eZ\Publish\Core\FieldType\Checkbox\Value,
* }>
*/
public function provideForValueIsNeverEmpty(): iterable
{
yield [new CheckboxValue(true)];
yield [new CheckboxValue(false)];
}

public function testEmptyValueIsEmpty(): void
{
self::markTestSkipped('Value of Checkbox fieldtype is never considered empty');
}
}