Skip to content

Conversation

@tedsalmon
Copy link

Description

Return empty string when the given date column is undefined in a grid

Fixed Issues (if relevant)

  1. Undefined Date Columns in Grid Default to current date / time #12461: Undefined Date Columns in Grid Default to current date / time

Manual testing scenarios

  1. Select a date column that has undefined values in a grid
  2. Verify that undefined values are showing up as empty string and not the current date and time

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 28, 2017

CLA assistant check
All committers have signed the CLA.

@mzeis
Copy link
Contributor

mzeis commented Nov 28, 2017

Hi,

thank you for your PR! Please can you have a look at the failing travis build? Thanks!

@mzeis mzeis self-assigned this Nov 28, 2017
@magento-engcom-team magento-engcom-team added Fixed in 2.3.x The issue has been fixed in 2.3 release line Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Nov 28, 2017
@tedsalmon
Copy link
Author

@mzeis,

Good catch! I wasn't familiar with the JS linter. The build should go 💚 soon and I've already rebased to squash the commits.

Thanks!
-Ted

@mzeis mzeis added this to the November 2017 milestone Nov 28, 2017
@mzeis
Copy link
Contributor

mzeis commented Nov 28, 2017

Great, thanks for the quick fix! I'll let you know if the interal tests show any problems.

The CET indicated in #12461 that this is fixed in 2.3-develop. As the code is still the same in https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Ui/view/base/web/js/grid/columns/date.js#L39, can you confirm if this is really true?

@ihor-sviziev ihor-sviziev self-assigned this Nov 28, 2017
@tedsalmon
Copy link
Author

@mzeis,

I can confirm that the linked fix works correctly, so I guess that makes my PR useless :)

FYI @ihor-sviziev

Thanks!
-Ted

@mzeis
Copy link
Contributor

mzeis commented Nov 28, 2017

@tedsalmon I'll ask around to find out how the problem was fixed in 2.3-develop. If it's a small change it could possibly fixed in 2.2.x too (unless 2.3 is good enough for you).

@tedsalmon
Copy link
Author

@mzeis,

The fix should just be this portion of the 2.3 code && value[this.index]. If it doesn't matter, I'll just backport that for this PR, since it's a cleaner solution than mine.

Thanks!
-Ted
cc @ihor-sviziev

@mzeis
Copy link
Contributor

mzeis commented Nov 28, 2017

@tedsalmon and me found out this is a duplicate of #11749 so I'm closing this one. It will be fixed in 2.2.2.

@mzeis mzeis closed this Nov 28, 2017
@tedsalmon tedsalmon deleted the issue_12461 branch November 28, 2017 21:03
@ihor-sviziev
Copy link
Contributor

FYI this issue was also fixed in 2.1-develop branch in #12033

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

Labels

bugfix duplicate Fixed in 2.3.x The issue has been fixed in 2.3 release line Progress: reject Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants