Add Logout Feature + Sessions#159
Conversation
… for pipelineing #30
nelsonic
added
the
in-progress
nelsonic
force-pushed
the
logout-endpoint-issue-158
branch
from
b6c15f5 to
1f8ddf3
Compare
Codecov Report
@@ Coverage Diff @@
## main #159 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 23 +1
Lines 529 554 +25
=========================================
+ Hits 529 554 +25
Continue to review full report at Codecov.
|
nelsonic
added
awaiting-review
| end | ||
| # defmodule Auth.Mailer do | ||
| # use Swoosh.Mailer, otp_app: :auth | ||
| # end |
There was a problem hiding this comment.
Compiling the project returns the following warning and error:
I think there are more things to comment or remove to make sure Swoosh is removed.
Looking at dwyl/phoenix-chat-example#66 to see what I've done to remove it
There was a problem hiding this comment.
It works fine on a fresh git clone. I think it might have been a conflict in my dependencies with another branch.
However swoosh is still defined in mix.lock file. If we want to remove the dependency completely we can run mix deps.clean --unused --unlock
| @@ -1 +1 @@ | |||
| web: mix phx.server No newline at end of file | |||
| web: MIX_ENV=prod mix ecto.migrate && mix assets.deploy && mix phx.server No newline at end of file | |||
There was a problem hiding this comment.
I'm not sure if we need to run assets.deploy in Procfile.
The elixir_buildbpack file run this command already:
https://github.com/dwyl/auth/blob/main/elixir_buildpack.config
There was a problem hiding this comment.
@SimonLab perfect. I was in doubt that it was working. but if you've confirmed it, let's remove it. 👍
| |> assign(:sid, session_id) | ||
| |> Auth.Session.end_session() |
There was a problem hiding this comment.
Here the sid is added to conn so the end_session function can update the end session value. The sid is then removed from the conn.
Instead of passing conn to the function I think we can pass the session_id directly. We'll need to udpate also the parameter for end_session and update_session_end but I think it makes things a be more easy to read and we'll be able to remove |> assign(:sid, session_id)
There was a problem hiding this comment.
That seems like a sensible refactor. The only reason I was doing adding the sid to conn was for pipeline. But if instead we make the session_id the second parameter of Auth.Session.end_session then it will be much cleaner. 💭
There was a problem hiding this comment.
I think we can even remove completely the conn and just have one argument. I'll check this on a new branch
Co-authored-by: Simon <simon.labondance@gmail.com>
|
Not sure why a conflict appears with the latest commit: I'll delete the spaces |
SimonLab
left a comment
SimonLab
left a comment
There was a problem hiding this comment.
Sessions looks good, thanks.
I still find the auth_controller a bit long and not as easy to read.
Hopefully the next PRs will make this easier to navigate
|
@SimonLab yeah, the auth_controller is definitely too big and could easily be split/simplified: https://github.com/dwyl/auth_plug/issues/40 |
3 participants
app_id,auth_providerandperson_id(ref) to sessions table for Create/logoutendpoint #158 / rel: [Epic] Session Management #30insert_sessionend_sessionasync: trueto speed up execution. ✅/logoutfeature/api/logoutendpoint that can be invoked from any "consumer" app viaAuthPlug.logout/1