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

PR: AUTH_API_KEY issue #42 #43

Merged
merged 191 commits into from
May 12, 2020
Merged

PR: AUTH_API_KEY issue #42 #43

merged 191 commits into from
May 12, 2020

Conversation

nelsonic
Copy link
Member

@nelsonic nelsonic commented Apr 1, 2020

This PR is quite epic.
It turned out to be a bit more code than I was expecting.
But I'm quite happy with the end result.
The code is definitely not perfect by any means.
But it has complete tests 💯 which means we can refactor if we need to!
The main thing is that it works.
So we can get back to building the features that people using our app actually care about!

What's included in this PR?

@nelsonic nelsonic added the in-progress An issue or pull request that is being worked on by the assigned person label Apr 1, 2020
@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #43 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #43   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          210       210           
=========================================
  Hits           210       210           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3dff03...a3dff03. Read the comment docs.

@@ -1,65 +0,0 @@
exports.config = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Brunch no longer used. #40

@nelsonic nelsonic mentioned this pull request Apr 1, 2020
@nelsonic nelsonic changed the title DWYL_API_KEY issue #42 PR: DWYL_API_KEY issue #42 Apr 1, 2020
@nelsonic nelsonic self-assigned this Apr 1, 2020
@nelsonic nelsonic mentioned this pull request May 7, 2020
13 tasks
@nelsonic
Copy link
Member Author

nelsonic commented May 8, 2020

@SimonLab I have finished writing the tests (and code) for the Registration/Login with Email+Password feature #63
(off-the-record: I regret adding this feature to this PR as it wasn't absolutely necessary and ended up taking way more time than I expected for some reason! but it's done now ...)

I'm not adding any more commits to this PR until you have completely reviewed it.
Ideally we won't add any more features to the PR and instead will add enhancements (of which there are many in the backlog!) in subsequent iterations.

Please review and share your thoughts when you can. (Monday at this stage).
Thanks! 👍

@nelsonic

This comment has been minimized.

Comment on lines +106 to +109
|> Map.put(:username, profile.login)
|> Map.put(:givenName, profile.name)
|> Map.put(:picture, profile.avatar_url)
|> Map.put(:auth_provider, "github")
Copy link
Member

Choose a reason for hiding this comment

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

I think we might be able to use Map.merge here

Copy link
Member Author

Choose a reason for hiding this comment

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

@SimonLab yes, definitely. 👍

Copy link
Member

@SimonLab SimonLab left a comment

Choose a reason for hiding this comment

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

Code makes sense 👍
Thanks for pairing on my api key issue @nelsonic

@SimonLab SimonLab merged commit 9744fe5 into master May 12, 2020
@SimonLab SimonLab deleted the DWYL_API_KEY-issue#42 branch May 12, 2020 20:42
SimonLab added a commit that referenced this pull request May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants