Skip to content

Fix the case where username is nil, not whodunnit#3210

Merged
mshibuya merged 2 commits into
railsadminteam:masterfrom
okuramasafumi:patch-1
Nov 4, 2019
Merged

Fix the case where username is nil, not whodunnit#3210
mshibuya merged 2 commits into
railsadminteam:masterfrom
okuramasafumi:patch-1

Conversation

@okuramasafumi

Copy link
Copy Markdown
Contributor

I encounter the problem where paper_trails's whodunnit isn't displayed.
It turned out that username method returns nil.
try method returns nil when there's no method, so when user doesn't have email method,
@user_class.find(@version.whodunnit).try(:email) returns nil.
Then, try this out:
nil rescue nil || :foo
This returns nil, surprisingly.
This means if user doesn't have email method, username is always nil.
So my fix is like below:
(nil rescue nil) || :foo
This returns :foo as expected.

@mshibuya

Copy link
Copy Markdown
Member

Could you write a spec for this issue?

@okuramasafumi

Copy link
Copy Markdown
Contributor Author

@mshibuya I'd like to add tests but CI seems to fail. Could you fix it?

@mshibuya

mshibuya commented Nov 1, 2019

Copy link
Copy Markdown
Member

It's already fixed in the master.
#3207

@okuramasafumi

Copy link
Copy Markdown
Contributor Author

@mshibuya Great, thanks!

I encounter the problem where paper_trails's whodunnit isn't displayed.
It turned out that `username` method returns nil.
`try` method returns nil when there's no method, so when user doesn't have `email` method,
`@user_class.find(@version.whodunnit).try(:email)` returns nil.
Then, try this out:
`nil rescue nil || :foo`
This returns nil, surprisingly.
This means if user doesn't have email method, username is always nil.
So my fix is like below:
`(nil rescue nil) || :foo`
This returns `:foo` as expected.
@mshibuya mshibuya merged commit 64e46b1 into railsadminteam:master Nov 4, 2019
@mshibuya

mshibuya commented Nov 4, 2019

Copy link
Copy Markdown
Member

Thanks!

@okuramasafumi okuramasafumi deleted the patch-1 branch November 5, 2019 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants