-
-
Notifications
You must be signed in to change notification settings - Fork 499
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
Fix dashboard recently added useers (#3473) #3529
Fix dashboard recently added useers (#3473) #3529
Conversation
Thank you @Isaac-alencar! This will make it easier for us to do some of our support tasks regarding newly added users. Just to manage expectations -- due to the holidays, it might be next week before someone gets around to reviewing this (or it might be today - I can't be sure). |
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.
Looks great! I added a couple of suggestions.
app/views/admin/dashboard.html.erb
Outdated
<%= link_to user.email, edit_admin_user_path(user), class: "users-list-name" %> | ||
<% else %> | ||
<%= link_to user.name, edit_admin_user_path(user), class: "users-list-name" %> | ||
<% end %> |
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.
This can be bunched together to be one line:
<%= link_to user.name.presence || user.email, edit_admin_user_path(user), class: "users-list-name" %>
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.
I decided to create a helper in order to do it, since we have a default value for the name field. So, when the name it is equal to "Name Not Provided" we display email otherwise the real name
@dorner thanks for reviewing! I'll fix it in a bit |
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.
@Isaac-alencar just one minor suggestion - otherwise it looks good!
app/helpers/dashboard_helper.rb
Outdated
@@ -40,6 +40,10 @@ def future_distributed | |||
number_with_delimiter future_distributed_unformatted | |||
end | |||
|
|||
def recently_added_user_display_text(user) | |||
(user.name.presence == "Name Not Provided") ? user.email : user.name |
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.
You don't need presence
here because nil
will by definition not equal the given string :) You also don't need the additional parentheses.
user.name == 'Name Not Provided' ? user.email : user.name
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.
Done! 🚀
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.
You don't need
presence
here becausenil
will by definition not equal the given string :) You also don't need the additional parentheses.user.name == 'Name Not Provided' ? user.email : user.name
It seems like the lint is failing without the parentheses
d7819e5
to
403e3cf
Compare
Awesome, thanks! |
Hey @Isaac-alencar -- Just letting you know this is in production now. Congratulations on your first human-essentials contribution that has made it to the real world! |
Resolves #3473
Description
In this PR, I fixed the superuser dashboard section "Recently added users" to display an email instead of "No Provided Name" and limited the total users displayed in this section to 20. Also, now the email text it is a link to edit user path as required in the issue.
Type of change
How Has This Been Tested?
Tests are located in
admin_request_spec.rb
from lines 21 to 36Screenshots