-
Notifications
You must be signed in to change notification settings - Fork 2
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
Simplified Employer Search #17
Conversation
23bf1ed
to
26dbfe7
Compare
6b5568b
to
d98105b
Compare
Add permissive data filter Allow clearing Only include verified/mappe Updated WIP Use Hotwire Switch to cards Alt formaction Fix modal bug Remove unused Fixes USWDS initialization bug Rework the form
cc5606c
to
ab5c530
Compare
parsed = JSON.parse(res) | ||
|
||
parsed['results'] | ||
end |
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.
Would be good to move this stuff into the ArgyleService at some point.
@@ -1,6 +1,4 @@ | |||
class Api::ArgyleController < ApplicationController | |||
skip_before_action :verify_authenticity_token |
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.
🔒
|
||
def search_params | ||
params.permit(:query) | ||
end |
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.
You didn't actually have to do this since the params aren't being passed into an ActiveRecord model, and you're extracting exactly the param key you want, but I guess it never hurts!
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.
@tdooner oooh! I never know when to use #require
. Usually a Rails form wraps the params in a key named after the model or something, but that's now how this is used. Happy to remove.
@@ -0,0 +1,36 @@ | |||
// Borrowed from https://github.com/18F/identity-idp/blob/59bc8bb6c47402f386d9248bfad3c0803f68187e/app/javascript/packages/request/index.ts#L25-L59 |
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.
"borrowed", lol, make sure to give it back when you're done!
const response = await fetch(ARGYLE_TOKENS_REFRESH, { | ||
method: 'post', | ||
headers: { | ||
'X-CSRF-Token': CSRF.token, |
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.
Nice. Rails used to have a way to do this out-of-the-box, but I googled and it looks like it was deprecated in Rails 7. If we end up making a lot of these fetch requests, maybe there is some way we can abstract this.
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.
Login.gov (lol) has a pretty good library for this but it's not published. Could "crib" it instead
Co-authored-by: Tom Dooner <[email protected]>
3c24490
to
90d17e3
Compare
This PR does a few things. You can see it broken down by commit:
Use CSRF. This commit re-enables CSRF token for the Argyle refresh token API endpoint. This is a security enhancement.
After that commit, I implement a new employer search that leverages Rails' Hotwire feature:
c2901ac