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.

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

Levy iss01 #2

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Levy iss01 #2

wants to merge 5 commits into from

Conversation

PatrickLevy
Copy link

Summary:
This branch contains a bug fix for issue-1, missing username in home view. The home view always indicated "unknown user" regardless of who was logged in.

Fix:
Lines 39 and 40 of /packages/login/client/templates.html incorrectly referenced the username. The following substitution was made: {{userName}} was altered to read {{currentUser.profile.name}}

@fletchtj
Copy link
Owner

fletchtj commented Apr 9, 2015

Your fix would solve the problem, but it then presents the problem that you point out in issue #3. I'd like you to take another shot at this one. The changes you commit to the branch will be picked up by the pull request automatically, so you don't need to do anything beyond committing the new code and pushing to your remote branch.

Here are a few hints:

  1. The reference to {{userName}} on lines 39 and 40 is different than a reference to {{username}} which might have been assumed. (It's my fault for naming that template helper the way I did...maybe a better name for the helper would have been userDisplayName.)
  2. Sometimes it is helpful to do a search through the project files for similar references.
  3. If needed, go back and read the original background information again for clues.

@PatrickLevy
Copy link
Author

Description

I found what I believe is a fix to the bug that results in incorrect or missing user profile information. Using console.log() statements I found that the "userId" and "user" values in the userName object were undefined upon being called. I noticed this yesterday too but was unsure of myself at the time. By updating Meteor.users.findOne(userId) to Meteor.users.findOne(Meteor.userId), the userName object seems able to locate the profile information for the user correctly.

If this is indeed the correct fix, then I think that dashboard.html should be updated to make use of the userName object as well, otherwise it will still result in missing profile information when an optional profile name is not given. (e.g. {{userName}} instead of {{currentUser.profile.name}}).

Further Concerns

While this seems to fix the problem, I am a bit unsure of myself as I see other bugs and am not sure if they are related. When a user logs out and logs back in under a different name, the profile information is not updated properly without a manual refresh of the page. A brief search on stackoverflow indicates that this may be due to Meteor.user().profile not being available until a moment after Meteor.user() becoming available.

http://stackoverflow.com/questions/14847575/meteor-user-profile-can-only-read-after-refresh

@fletchtj
Copy link
Owner

Right, so Meteor.user() and Meteor.userId() are reactive data sources, and depend on the currently logged in user. Meteor.userId() typically would return a value pretty quick, but may not be defined right away. Meteor.user() is a helper for that essentially returns the equivalent of Meteor.users.findOne(Meteor.userId()), so it has to grab the data from the database. The profile object is a property of the user object, so it won't be available until the user object is available.

I don't think we would want to make a change in the userName helper from Meteor.users.findOne(userId) to Meteor.users.findOne(Meteor.userId()) as this would limit the method to only returning the current user's username. By using the variable userId, you can pass in any user._id and get a username.

I also agree with your change from {{currentUser.profile.name}} to {{userName}}, so feel free to make that change.

@PatrickLevy
Copy link
Author

Detailed description of bug and possible cause:

  • Usernames are incorrectly displayed as "unknown user" on the views produced by "dashboard.html" and "templates.html."
  • Both of these views make a call to {{userName}}, but the userName function is being passed an "undefined" userId from these pages. The userName function is therefore unable to find the user's profile so it returns the value "unknown user."

Further observations:

  • The view produced by "users_index.html" was able to correctly display both the username and profile names for all the users. This page also utilizes the userName function, which demonstrates that the userName function is working properly and should not be altered.
  • The call to userName is made from this page with a reference to the user's id: {{userName _id}}
  • This page cycles through each user with a spacebars loop of {{#each users}}

Proposed fix:

  • Before making the call to userName, spacebars must be used to access the current user's profile: {{#with currentUser}}
  • Call the userName function with a reference to _id : {{userName _id}}

Testing:

  • I tested the program with three user profiles: the admin account, an account that included a username and an optional profile name, and an account that only included the username and no optional profile name.
  • All three accounts appear to produce the proper names on all pages of the program.
  • There is no manual refresh needed as indicated from the previous idea for a bug fix, further demonstrating that the previous fix was incorrect.
  • Using console.log(), I can verify that the userName function is now receiving valid data by which to locate the user's account information.

@fletchtj
Copy link
Owner

Well done! I might also suggest that you could alter the userName template helper to provide a default value to the Utils.objects.userName method, if a userId is not passed in. In that case, you could do something like the following starting on line 13 of packages/utils/template_helpers.js:

Template.registerHelper("userName", function (userId) {
    var userId = userId || Meteor.userId();
    return Utils.objects.userName(userId);
});

Then, you only need to pass in a userId when you are getting other user names. Default behavior would be to return the current user's name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants