-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add endpoint to save SUMA credentials #2271
Conversation
01637d2
to
6a47488
Compare
6a47488
to
e7838f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super! Just a few notes
test/trento_web/controllers/v1/suma_credentials_controller_test.exs
Outdated
Show resolved
Hide resolved
test/trento_web/controllers/v1/suma_credentials_controller_test.exs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same things Nelson advised plus a bonus tip
with {:ok, saved_settings} <- | ||
body_params |> Map.from_struct() |> SoftwareUpdates.save_settings() do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: just be careful with the pipes in with statements, they can become pretty unreadable fast (personal PTSD moment with that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Description
Adds endpoint to save SUMA credentials.
Using this endpoint, (new) credentials can only be saved if there currently aren't existing saved credentials.
How was this tested?
Added unit tests