Per 9985 entering 2fa incorrect code works#527
Conversation
67ab92d to
622de5e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
==========================================
+ Coverage 43.70% 43.75% +0.04%
==========================================
Files 366 366
Lines 11211 11215 +4
Branches 1840 1841 +1
==========================================
+ Hits 4900 4907 +7
+ Misses 6148 6144 -4
- Partials 163 164 +1 ☔ View full report in Codecov by Sentry. |
| data: any = {}, | ||
| responseClass?: ResponseClass<T>, | ||
| options: RequestOptions = defaultOptions, | ||
| responseType?: 'json' | 'text', |
There was a problem hiding this comment.
Thank you for adding this new feature to the HttpV2Service. I think ideally I'd like to see responseType in the options object instead of a positional argument. It makes our public methods have up to five arguments and some internal methods have even more. This can be annoying when we want to make a basic request with default options but we need to adjust the response type because we have to fill out every argument that would have been optional. (I regret having options come after resposneClass since those options are typically set more than the responseClass is).
| data: any = {}, | ||
| method: HttpMethod, | ||
| options: RequestOptions, | ||
| responseType: any = 'json', |
There was a problem hiding this comment.
I would like this typing to be narrowed down instead of being any type.
| data: any = {}, | ||
| responseClass?: ResponseClass<T>, | ||
| options: RequestOptions = defaultOptions, | ||
| responseType?: 'json' | 'text', |
There was a problem hiding this comment.
Since we're using this 'json' | 'text' typing everywhere, we should probably just define it as a specific type like
type ResponseType = 'json' | 'text';That way if we ever have to communicate with an external API that uses XML for instance we only have to change this typing on one line instead of many.
| return observable.pipe( | ||
| map((response: Object | Array<Object>) => { | ||
| if (responseType === 'text') { | ||
| return [response as unknown as T]; |
There was a problem hiding this comment.
This is a legitimate question we should probably think about... if the resposneType is text, we should always be returning a string instead of an object so response should just be able to be cast to string here. Of course, this function is expecting an observable of type T[]. I wonder if there's anyway to force T to be string when responseType is set to text.
There was a problem hiding this comment.
Good question. Maybe this could be a separate ticket? I have tried doing just that but I got many errors in different files unrelated to the ticket
| method: HttpMethod = 'post', | ||
| responseClass?: new (data: any) => T, | ||
| options: RequestOptions = defaultOptions, | ||
| responseType: 'json' | 'text' = 'json', // NEW: response type parameter |
There was a problem hiding this comment.
We do not need that comment.
|
@meisekimiu addressed your comments! Thank you! |
k8lyn6
left a comment
There was a problem hiding this comment.
This fix does resolve the issue (submitting an incorrect code just takes you back to the beginning). However, I don't think this is the best user experience, as it might still be a little confusing. It would be a better user experience if entering an incorrect code gives you an error and allows you to try entering the code again.
@meisekimiu would this be doable with this ticket, or does it require backend work? If it requires work outside of the ticket, I'll go ahead and approve this and we can create additional tickets.
|
I believe there would be some additional backend work required to do this properly. |
k8lyn6
left a comment
There was a problem hiding this comment.
Thanks @meisekimiu . I'll create an additional ticket and go ahead and mark this as approved. Thanks @crisnicandrei !
Add api call after removing a method
c3bdaaa to
8dece3d
Compare
This pull request fixes the bug that occurs when entering an incorrect code when removing a multi factor method
Steps to test:
@meisekimiu Do you think this implementation will do? Or should we change the backend as I mentioned on Slack?