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

Code review #75

Merged
merged 8 commits into from
May 14, 2020
Merged

Code review #75

merged 8 commits into from
May 14, 2020

Conversation

SimonLab
Copy link
Member

ref: #74

@SimonLab SimonLab added the in-progress An issue or pull request that is being worked on by the assigned person label May 13, 2020
@SimonLab SimonLab self-assigned this May 13, 2020
@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #75 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #75   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          210       207    -3     
=========================================
- Hits           210       207    -3     
Impacted Files Coverage Δ
lib/auth_web/router.ex 100.00% <ø> (ø)
lib/auth/apikey.ex 100.00% <100.00%> (ø)
lib/auth/email.ex 100.00% <100.00%> (ø)
lib/auth/person.ex 100.00% <100.00%> (ø)
lib/auth_web/controllers/apikey_controller.ex 100.00% <100.00%> (ø)
lib/auth_web/controllers/auth_controller.ex 100.00% <100.00%> (ø)

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 91a6ae1...25fcb02. Read the comment docs.


keys = Auth.Apikey.list_apikeys_for_person(person.id)
assert length(keys) == 3
end

#
# alias Auth.Ctx
Copy link
Member

Choose a reason for hiding this comment

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

we can probably just delete this. 👍

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@SimonLab these updates make sense.
When ready, please assign. 👍

@SimonLab SimonLab assigned nelsonic and unassigned SimonLab May 14, 2020
@SimonLab SimonLab added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels May 14, 2020
@nelsonic nelsonic added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels May 14, 2020
givenName: profile.name,
picture: profile.avatar_url,
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.

👍

person # |> IO.inspect(label: "updated person:230")
{:ok, person} =
changeset(%Person{id: ep.id}, merged)
# |> IO.inspect(label: "changeset transformed:234")
Copy link
Member

Choose a reason for hiding this comment

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

I'm happy for all IO.inspect to be removed.
Looking forward to adding more advanced Logging dwyl/app#274 👍

conn # email invalid, re-render the login/register form:
|> index(params)
# email invalid, re-render the login/register form:
index(conn, params)
Copy link
Member

Choose a reason for hiding this comment

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

makes sense. dunno why I left this redundant pipe in.
It has another function before invoking index.
Thanks for tidying.


end
# setup password form depending on person values
defp password_form(conn, person, state) do
Copy link
Member

Choose a reason for hiding this comment

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

this is a good split out. thanks. 👍

# |> IO.inspect(label: "int")

assert decrypted == person_id
end

test "decode_decrypt/1 reverses the operation of encrypt_encode/1" do
person_id = 4869234521
person_id = 4_869_234_521
Copy link
Member

Choose a reason for hiding this comment

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

did dialyxir update the format of these numbers?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might have been mix format maybe

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Good to know they both work. 👍
person_id should be an unsigned int without the underscores because it's defined by PostgreSQL which does not use underscores in integers.
I will update the code in the future to reflect this. 👍

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@SimonLab thanks for making these improvements. 👍

@nelsonic nelsonic merged commit 07d3420 into master May 14, 2020
@nelsonic nelsonic deleted the code-review-#74 branch May 14, 2020 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review Issue or pull request that is currently being reviewed by the assigned person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants