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

Jump to bottom

EZP-30712: Fixed reverseTransform for midnight value #301

Merged
merged 4 commits into from
Jul 26, 2019
Merged

EZP-30712: Fixed reverseTransform for midnight value #301

merged 4 commits into from
Jul 26, 2019

Conversation

SerheyDolgushev
Copy link
Contributor

@SerheyDolgushev SerheyDolgushev commented Jul 1, 2019 •
edited by micszo
Loading

JIRA: https://jira.ez.no/browse/EZP-30712

Can't set a time to be midnight without this fix

@micszo
Copy link
Member

micszo commented Jul 2, 2019

Hi @SerheyDolgushev, thanks for opening the PR! Could you change base and rebase to 2.5?

@micszo micszo requested review from webhdx, alongosz and Nattfarinn July 2, 2019 12:42
@alongosz alongosz changed the title Fix midnight time EZP-30712: Fixed reverseTransform for midnight value Jul 2, 2019
alongosz
alongosz requested changes Jul 2, 2019
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

@SerheyDolgushev is it possible to cover this with some minimal unit tests? I'd like to know what empty-ish value occurs for midnight w/o debugging it myself (good input data for test case) ;)

Seems we're missing tests for TimeValueTransformer so please add minimal coverage of just testTransform with a few inputs. Please see how it's done in \EzSystems\RepositoryForms\Tests\FieldType\DataTransformer\DateIntervalToArrayTransformerTest.

Besides as mentioned above, this is a bug so should go to branch 2.5.

CS nitpick:

lib/FieldType/DataTransformer/TimeValueTransformer.php Outdated Show resolved Hide resolved
@SerheyDolgushev SerheyDolgushev changed the base branch from master to 2.5 July 23, 2019 07:23
@SerheyDolgushev
Copy link
Contributor Author

Requested CS changes and rebase against 2.5 is done.

TimeValueTransformer uses eZ\Publish\Core\FieldType\Time\Value, which is not the part of this package. So there is no easy way to test TimeValueTransformer. @alongosz can you please suggest a solution here?

@alongosz
Copy link
Member

This is still unresolved:

@SerheyDolgushev is it possible to cover this with some minimal unit tests? I'd like to know what empty-ish value occurs for midnight w/o debugging it myself (good input data for test case) ;)

Seems we're missing tests for TimeValueTransformer so please add minimal coverage of just testTransform with a few inputs. Please see how it's done in \EzSystems\RepositoryForms\Tests\FieldType\DataTransformer\DateIntervalToArrayTransformerTest.

I'm sorry, but I don't have time to reproduce this myself and decide if the fix is correct based on behavior. This is why tests are helpful.

@SerheyDolgushev
Copy link
Contributor Author

@alongosz please review 0e9d3b3.

alongosz
alongosz reviewed Jul 26, 2019
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

@SerheyDolgushev Almost good, one final remark from my side:

tests/RepositoryForms/FieldType/DataTransformer/TimeValueTransformerTest.php Outdated Show resolved Hide resolved
tests/RepositoryForms/FieldType/DataTransformer/TimeValueTransformerTest.php Show resolved Hide resolved
@katarzynazawada katarzynazawada self-assigned this Jul 26, 2019
@lserwatka lserwatka merged commit e76a1b7 into ezsystems:2.5 Jul 26, 2019
@lserwatka
Copy link
Member

@alongosz could you merge it up?

@alongosz
Copy link
Member

@lserwatka done.

Thank you @SerheyDolgushev 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katarzynazawada katarzynazawada katarzynazawada approved these changes

@webhdx webhdx webhdx approved these changes

@alongosz alongosz alongosz approved these changes

@Nattfarinn Nattfarinn Awaiting requested review from Nattfarinn

Assignees

@katarzynazawada katarzynazawada

Labels
Bug QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SerheyDolgushev @micszo @alongosz @lserwatka @webhdx @katarzynazawada