Add flow for existing users to accept new policies - #1160
Conversation
|
Deployment previews on netlify for branch
|
| const { data } = await axios.get('/v1/policies/current') | ||
| return data.items | ||
| } | ||
| export const getMissingPolicies = async () => { |
There was a problem hiding this comment.
getMissingPolicies and getCurrentPolicies looks similar (same response shape), only the path differs.
You can consider writing getPolicies(path) or something like getPoliciesByStatus('missing' |'current') ti reduce repetition.
| const { data } = await axios.get('/v1/policies/missing') | ||
| return data.items | ||
| } | ||
| export const acceptPolicies = async (policyIds) => { |
There was a problem hiding this comment.
acceptPolicies doesn't return anything. It can return response.data or something
| } | ||
| } | ||
|
|
||
| const state = getDefaultState() |
There was a problem hiding this comment.
exporting a singleton state object is a bit off to me. Does it lead to a shared state across store instances?
|
|
||
| const getters = { | ||
| hasLatestError: state => !!state.latestError, | ||
| latestError: state => state.latestError, |
There was a problem hiding this comment.
The action name latestError conflicts conceptually with the getter latestError, which makes usage and debugging confusing (dispatch('latestError') vs getters.latestError). Rename the action to something imperative like reportLatestError or setLatestError to clarify intent.
| try { | ||
| commit('setMissingPolicies', await api.getMissingPolicies()) | ||
| } catch (error) { | ||
| commit('failedToGetMissingPolicies', true) |
There was a problem hiding this comment.
The failedToGetMissingPolicies flag is set to true on error. But if I understand correctly, it is never reset to false on succesful fetch?
| commit('setMissingPolicies', await api.getMissingPolicies()) | ||
| } catch (error) { | ||
| commit('failedToGetMissingPolicies', true) | ||
| dispatch('latestError', { |
There was a problem hiding this comment.
The dispatch('latestError', ...) call is not awaited or returned. Consider return await dispatch('latestError', ...) (or at least await)
| hasMissingPolicies: function () { | ||
| return this.$route.meta.requiresAuth && | ||
| !this.$route.meta.requiresAuth.excludeFromPolicyChecks && | ||
| this.$store.getters.missingPolicies?.length |
There was a problem hiding this comment.
I don't really get this bit...
hasMissingPolicies returns missingPolicies?.length, which is a number (or worst case undefined). Since this is used in v-if, is it less error-prone to return an explicit boolean (for example, (this.$store.getters.missingPolicies?.length ?? 0) > 0)?
| onContactForm () { | ||
| this.$router.push('/contact') | ||
| }, | ||
| async onAccept () { |
There was a problem hiding this comment.
inFlight is set back to false only on the success path. If acceptPolicies throws/rejects or fails in general, inFlight will remain true and the UI will be stuck.
try/catch/finally can wrap the dispatch so inFlight is always reset
| Please accept the updated | ||
| <PolicyList | ||
| :policies="policies" | ||
| article="" |
There was a problem hiding this comment.
article is empty, if the prop is optional, omit it
| ) | ||
| await this.$store.dispatch('acceptPolicies', policyIds) | ||
| this.inFlight = false | ||
| }, |
There was a problem hiding this comment.
Optional:
Add tests for the modal actions:
- clicking “Contact Form” navigates to
/contact - “Continue” is disabled until the checkbox is checked
onAcceptre-enables the UI even whenacceptPoliciesrejects (error path).
2 participants
Bug: T401223