feat(davinci-client): mfa otp field support#262
Conversation
🦋 Changeset detectedLatest commit: e38b49e The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit e38b49e.
☁️ Nx Cloud last updated this comment at |
|
Deployed b3df0d6 to https://ForgeRock.github.io/ping-javascript-sdk/pr-262/b3df0d65173ef64ec5af80c74931ac2d991fe858 branch gh-pages in ForgeRock/ping-javascript-sdk |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
+ Coverage 47.35% 55.50% +8.15%
==========================================
Files 29 20 -9
Lines 1472 1380 -92
Branches 148 163 +15
==========================================
+ Hits 697 766 +69
+ Misses 775 614 -161
🚀 New features to boost your workflow:
|
|
@cerebrl changeset bot is having an issue detecting your changeset, could you manually remove it, then push it back up as its own commit so that maybe the force push doesn't mess up its commit tracking? |
|
|
||
| if (collector.type === 'DeviceRegistrationCollector') { | ||
| if (typeof action.payload.id !== 'string') { | ||
| throw new Error('Index argument must be a string'); |
There was a problem hiding this comment.
Are we wanting to throw errors? if so - should this throw a TypeError?
| * @param {ObjectValueCollectorTypes} [collectorType] - Optional type of the ObjectCollector. | ||
| * @returns {ObjectCollector} The constructed ObjectCollector object. | ||
| */ | ||
| export function returnObjectCollector< |
There was a problem hiding this comment.
Not blocking by any means, but in the spirit of our discussion of a pipeable api,
we could probably refactor this and similar functions like returnActionCollector and returnSingleValueCollector to be this type of api, and reduce duplication in checking the errors.
I think we can abstract the error checking as their own functions and we can then have a "switch" type function for the device return
pipe(
field,
checkForKeyInField,
checkForLabelInField,
checkForTypeInField,
mapTypeToDevice
)
How we want to handle the "error" channel here is up to us, but I think if we wanted an api in this manner, these style internal functions have very similar structures that can be re-used in a pipeline roughly like this
Just leaving as food for thought, not required for this PR.
| required: boolean; | ||
| }; | ||
|
|
||
| export type DaVinciField = |
There was a problem hiding this comment.
As this "type" grows, I wonder if we should consider breaking this into some groups of subtypes.
Not blocking for this PR, and I do think this organization has pro's but also negatives as we have to think about where things go and this is exposed to the consumer, so we'd want to document this well so a consumer knows what these fields are when dealing with more "primitive" types to the group.
i.e. DeviceFields | InputFields | ValidationFields | RedirectFields (naming subject to change
There was a problem hiding this comment.
I agree with this. I'll do it now.
|
|
||
| if (collector.type === 'DeviceAuthenticationCollector') { | ||
| if (typeof action.payload.id !== 'string') { | ||
| throw new Error('Index argument must be a string'); |
There was a problem hiding this comment.
TypeError? What is the thought process in returning a error here versus our current idea of not returning errors?
Since we aren't catching this we are returning this to the user here, is this a new pattern?
There was a problem hiding this comment.
As mentioned on the call, this is not a new pattern, and the thrown error doesn't actually make it to the user. The original caller catches it, but I agree with your overall sentiment. We should revisit this and see if there's a better pattern.
| export function returnObjectCollector< | ||
| Field extends DeviceAuthenticationFieldValue | DeviceRegistrationFieldValue, | ||
| CollectorType extends ObjectValueCollectorTypes = 'ObjectValueCollector', | ||
| >(field: Field, idx: number, collectorType: CollectorType) { |
There was a problem hiding this comment.
I'm wondering if we should restructure how across the board we are doing function arguments to be composable.
I just think how can we use these functions within a pipe and it's pretty difficult.
There was a problem hiding this comment.
Yeah, I agree. As to not make this into a large refactor, let's save these changes until the implementation of our new "pipe" solution.
3 participants
JIRA Ticket
SDKS-3901
Description
Adds MFA OTP (email, sms, voice) to field support.
Changeset: Yes!