dwyl / mvp

πŸ“² simplest version of the @dwyl app
https://mvp.fly.dev
GNU General Public License v2.0
84 stars 2 forks source link

PR: DRY Assign JWT to Socket Code issue #215 #225

Closed DuldR closed 1 year ago

DuldR commented 1 year ago

This PR adds in application of auth_plug. Both the documentation within BUILDIT and auth_controller itself have been updated.

Question: @nelsonic I saw you stated both instructions needed to be updated in the BUILDIT, but I didn't see where else this function was used. Were you referring to the second on_mount function at auth_controller? If so, I didn't know if I needed to add the pattern matching for the jwt there as well.

I think there's several blockers on this one. We've got two failing tests related to this change.

  1. We're using auth_plug 1.4.14 in our mix.exs at https://github.com/dwyl/mvp/blob/78c06aea16d1436f08af3ac7da6578a70423ff38/mix.exs#L63. This does not include the new assign_jwt_to_socket/3 function. We'll need to update to 1.5.1 to get access to it.
  2. Updating auth_plug to 1.5.1 results in a dependency loop: image updating envar image

Can those be updated at this time? I didn't go further than the fields dependency.

codecov[bot] commented 1 year ago

Codecov Report

Merging #225 (69a4b78) into main (78c06ae) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 69a4b78 differs from pull request most recent head fd87974. Consider uploading reports for the commit fd87974 to get more accurate results

@@            Coverage Diff            @@
##              main      #225   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          312       312           
=========================================
  Hits           312       312           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

nelsonic commented 1 year ago

Hey @DuldR, πŸ‘‹ Thank you for opening this PR! πŸŽ‰ The code you've added (and removed) is exactly what is needed. πŸ‘Œ No need to update anything in the second on_mount function as it matches the cases where there is no JWT in the session.

Annoying about the "dependency loop". πŸ™„ Luckily we "control" both of those other dependencies so we can easily "fix" this. βœ… Please mark the PR as "Read for review" and assign to me. πŸ™

nelsonic commented 1 year ago

Looking into why the Build refused to pass. Guessing it's the auth_plug update issue described above. πŸ’­

nelsonic commented 1 year ago

Looks like I'm going to have to update a couple of deps ... BRB! ⏳

nelsonic commented 1 year ago

GOTO: #226