-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Dashboard width revision message should handle nil #39710
Dashboard width revision message should handle nil #39710
Conversation
Even though there shouldn't be `nil` values in dashboard.width, because the migration populates w/ the default value of `full`, it is still true that because of the version before the migration, revisions can have `nil` for the width value. The diff string impl didn't consider this, causing an NPE with `(name nil)`.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change looks good, but can we please get a test for this? We like tests in these here parts
@camsaul, I made a slight modification and added a test. It's probably still ok, but a quick second look might be good, if you have a second. |
@adam-james-v Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
* Revision Diff Should handle `nil` Dashboard Width Even though there shouldn't be `nil` values in dashboard.width, because the migration populates w/ the default value of `full`, it is still true that because of the version before the migration, revisions can have `nil` for the width value. The diff string impl didn't consider this, causing an NPE with `(name nil)`. * Use :guard to prevent nil values from matching * Add test checking that dashboard width change is described correctly
* Revision Diff Should handle `nil` Dashboard Width Even though there shouldn't be `nil` values in dashboard.width, because the migration populates w/ the default value of `full`, it is still true that because of the version before the migration, revisions can have `nil` for the width value. The diff string impl didn't consider this, causing an NPE with `(name nil)`. * Use :guard to prevent nil values from matching * Add test checking that dashboard width change is described correctly Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>
Fixes: #38910
Even though there shouldn't be
nil
values in:width
keys on the dashboard, (because the migration populates w/ the default value offull
), it is still true that because of the version before the migration, revisions can havenil
for the widthvalue. The diff string impl didn't consider this, causing an NPE with
(name nil)
.This change should guard the diff string from trying to run on these
nil
values.