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.

Sign up for GitHub

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

Jump to bottom

Implement SetTimberUserContext plug #1101

Merged
merged 9 commits into from
Dec 11, 2017
Merged

Implement SetTimberUserContext plug #1101

merged 9 commits into from
Dec 11, 2017

Conversation

tteerawat
Copy link
Member

@tteerawat tteerawat commented Oct 22, 2017 •
edited by code-corps bot

What's in this PR?

  • Create CodeCorpsWeb.Plug.SetTimberUserContext plug module.
  • Implement User.full_name/1 function.
  • Remove unused code from CodeCorpsWeb.GuardianSerializer module.
  • Apply CodeCorpsWeb.Plug.SetTimberUserContext module to:
    • Router file in :current_user pipeline.

References

Fixes #932

@tteerawat tteerawat self-assigned this Oct 22, 2017
@tteerawat tteerawat changed the title Implement user context logger plug Implement SetTimberUserContext plug Oct 22, 2017
@tteerawat tteerawat force-pushed the implement-user-context-logger-plug branch from a0244b8 to ce0c621 Compare October 22, 2017 14:00
@joshsmith
Copy link
Contributor

@begedin would you mind giving this a look?

begedin
begedin previously approved these changes Dec 5, 2017
View reviewed changes
Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well tested and more explicit than what we have. Looks good to me.

@joshsmith
Copy link
Contributor

There are some conflicts here now, though. We've upgraded Guardian.

@teerawat1992 is it possible for you to deconflict the changes and bring them into the newly updated Guardian code?

@tteerawat
Copy link
Member Author

@joshsmith Sure, I'll deconflict the changes 🙇

@tteerawat tteerawat force-pushed the implement-user-context-logger-plug branch from ce0c621 to ca3141f Compare December 8, 2017 16:13
@tteerawat
Copy link
Member Author

@joshsmith @begedin I've rebased from develop branch and refactored Guardian module, please review again when you guys have time 🙇

begedin
begedin requested changes Dec 9, 2017
View reviewed changes
Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the typo is cleared, I'm fine with merging this in.

test/lib/code_corps/model/user_test.exs Outdated
changeset = User.changeset(%User{}, @valid_attrs)
assert changeset.valid?
describe "full_name/1" do
test "returns full name correclty" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got a typo here, sorry 😬

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops 😅

@tteerawat
Fix typo in user_test.exs
eef818e 
Change 'correclty' to 'correctly'.
@begedin begedin merged commit 917bb86 into develop Dec 11, 2017
@begedin begedin deleted the implement-user-context-logger-plug branch December 11, 2017 07:12
@begedin begedin mentioned this pull request Dec 11, 2017
Merged
@joshsmith
Copy link
Contributor

🙌 great work @teerawat1992!

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

@begedin begedin begedin approved these changes

@joshsmith joshsmith Awaiting requested review from joshsmith

Assignees

@tteerawat tteerawat

Labels
needs code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tteerawat @joshsmith @begedin