feat(callbacks): device-profile-node addition #121

ryanbas21 · 2023-05-11T01:33:24Z

add the device profile node to the login widget. Supports the ability to render a device profile node as its own component. It will work within a page node but it has some edge cases that are difficult to account for. Password managers may make it so an error state is not tracked appropriately while a device profile is being collected and the form will silently fail.

This is a known limitation at the moment.

https://bugster.forgerock.org/jira/browse/SDKS-2000

Screen.Recording.2023-05-25.at.10.15.04.AM.mov

ryanbas21 · 2023-05-11T01:47:19Z

Your preview environment pr-121-ryanbas21 has been deployed.

Preview environment endpoint is available here

cerebrl

Just leaving some comments since I was looking over the code. Can you remove all the files in the components/compositions and components/primitives directories. I think that's leftover from a misunderstanding of the story.

.svelte-kit/tsconfig.json

vitest.config.ts

vitest.config.ts.timestamp-1683768174756.mjs

src/lib/journey/stages/step.mock.ts

src/lib/journey/callbacks/device-profile/device-profile.svelte

src/lib/journey/callbacks/device-profile/device-profile.mock.ts

src/lib/journey/_utilities/metadata.utilities.test.ts

cerebrl · 2023-05-22T23:04:44Z

src/lib/journey/_utilities/metadata.utilities.test.ts

+        // because this is a a function now, i'm using the function defined so
+        // the memory is the same and its picked up as such by toEqual.


I think I know what you mean here, but could you reword and clean up the comment a bit for better clarity?

cerebrl · 2023-05-25T20:22:22Z

src/lib/journey/_utilities/metadata.utilities.test.ts

+        // because this is a a function now, i'm using the function defined so
+        // the memory is the same and its picked up as such by toEqual
+        // rather than creating a new function which would be different
+        // in memory.


If this comment is going to persist in our source code, let's ensure it's written well.

/** * Unlike the other properties, `isStepSelfSubmittable` is a function, * so for `toEqual` to pass, the "expected" object needs to reference * the same function as the `result` object. * * TODO: Modify this to assert against the returned value, rather than * relying on function reference. */

cerebrl · 2023-05-25T20:24:23Z

src/lib/journey/callbacks/device-profile/device-profile.stories.js

+  const location = cb.isLocationRequired();
+  const metadata = cb.isMetadataRequired();
+
+  const dvice = await device.getProfile({ location, metadata });


Rather than relying on a misspelling of "device", can we just call name this profile or deviceProfile?

cerebrl · 2023-05-25T20:29:24Z

tests/widget/inline/widget-device-profile.test.js

+
+import { asyncEvents } from '../../utilities/async-events.js';
+
+test('Inline widget with login in Canadian French', async ({ page, context }) => {


Looks like the test name needs rewording.

cerebrl · 2023-05-25T20:34:31Z

tests/widget/inline/widget-device-profile.test.js

+  await page.waitForRequest(/\/authenticate/, async () => {
+    const text = page.getByText('Collecting Profile');
+    await expect(text).toBeVisible();
+  });


I believe this is redundant as the clickButton already accounts for waiting until the request is resolved. Once clickButton itself resolves, you can test whatever you expect to be in the next view.

hmm idk this was required for me to get it working.

The device profile text disapeared too quickly.

I've pulled this in locally, and I believe I know what's happening. This block of code is never executed. So, the assertion is never applied, which gives you a false sense of "passing". The reason for this is the /authenticate request is already resolved within the clickButton method, which is awaited, so the following waitForRequest event listener is never triggered. If you write a console message within it, you'll never see it logged.

The reason it doesn't work when it's not wrapped in this waitForRequest is because the "Collecting Profile" message is never displayed by the UI, ever. It's not that it disappears to quickly. You can see that the message is missing in your GIF provided in the PR description.

That's due to the fact that the node in the journey uses message as the key. Keys have to be locales or languages, so the key would be en or en_US with the value of "Collecting Profile". Once you change the key to en you can then directly assert using the below without wrapping it in a waitFor*:

await expect(page.getByText('Collecting Profile')).toBeVisible();

cerebrl · 2023-05-25T20:37:35Z

tests/widget/inline/widget-device-profile.test.js

+    const text = page.getByText('Collecting Profile');
+    await expect(text).toBeVisible();
+  });
+  await context.grantPermissions(['geolocation']);


I feel like this test isn't finished. We don't seem to be asserting anything, just granting a permission. Can we write this so we assert something regarding device profile to ensure the profile is being generated?

cerebrl

I think this looks great overall. Great improvements. We do have a failed test, and accidential file commit, and some small changes, if you don't mind.

.vscode/launch.json

src/lib/journey/callbacks/device-profile/device-profile.svelte

cerebrl · 2023-06-01T21:06:06Z

src/lib/journey/stages/stages.stories.js

We should probably add a story within this that composes device profile with a standard login form ... Just to ensure it composes well without breaking.

Have you addressed this comment? I don't see this in the updated code.

tests/widget/inline/widget-device-profile.test.js

cerebrl

This looks good. I just have a question about the README and the missing change that addresses the comment I made a week or so ago about Storybook stage story to ensure device profile composes with a typical login UI without issue. You only have a stage story for Device Profile alone.

cerebrl · 2023-06-06T14:07:46Z

README.md

Is this change coming from Chris? I'm trying to understand why the README is so heavily changed.

cerebrl · 2023-06-06T14:43:20Z

package/CHANGELOG.md

+
+### Bug Fixes
+
+* **files:** files in package.json means it only updates whats in there ([bdc23ae](https://github.com/forgerock/forgerock-web-login-framework/commit/bdc23ae6c8030b84aea9e92db83a72bee442263a))


Since our commits are going to drive our CHANGELOG conent, we need to write better commits. This commit is an example of one that doesn't make much sense read in this log. Plus, there's a grammatical error, which doesn't look great.

cerebrl · 2023-06-06T15:20:36Z

src/lib/journey/stages/stages.stories.js

Have you addressed this comment? I don't see this in the updated code.

cerebrl

Thank you. Looks great.

cerebrl · 2023-06-08T13:44:52Z

Looks like we have a failure on one of the Interaction tests: https://www.chromatic.com/test?appId=64075ad87b9e160633c33298&id=64810969ad7b6340d675c048

ryanbas21 · 2023-06-08T15:30:23Z

Looks like we have a failure on one of the Interaction tests: https://www.chromatic.com/test?appId=64075ad87b9e160633c33298&id=64810969ad7b6340d675c048

yeah i fixed it and forgot to push, sorry

add the device profile node to the login widget. can use a device profile node in a page node (with small limitation of timing) or on its own.

ryanbas21 · 2023-06-12T15:43:49Z

🎉 This PR is included in version 1.1.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

ryanbas21 · 2023-07-27T17:15:08Z

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ryanbas21 temporarily deployed to Preview May 11, 2023 01:33 — with GitHub Actions Inactive

ryanbas21 temporarily deployed to Preview May 11, 2023 17:49 — with GitHub Actions Inactive

cerebrl requested changes May 11, 2023

View reviewed changes

ryanbas21 temporarily deployed to Preview May 11, 2023 20:01 — with GitHub Actions Inactive

ryanbas21 force-pushed the device-profile-node branch from 849ac8b to fda7a37 Compare May 11, 2023 21:12

ryanbas21 temporarily deployed to Preview May 11, 2023 21:13 — with GitHub Actions Inactive

cerebrl reviewed May 22, 2023

View reviewed changes

ryanbas21 temporarily deployed to Preview May 24, 2023 18:04 — with GitHub Actions Inactive

ryanbas21 force-pushed the device-profile-node branch from b4ce64a to 88b56dc Compare May 24, 2023 18:25

ryanbas21 temporarily deployed to Preview May 24, 2023 18:25 — with GitHub Actions Inactive

cerebrl requested changes May 25, 2023

View reviewed changes

ryanbas21 force-pushed the device-profile-node branch from 88b56dc to c09376e Compare June 1, 2023 18:57

ryanbas21 temporarily deployed to Preview June 1, 2023 18:57 — with GitHub Actions Inactive

ryanbas21 force-pushed the device-profile-node branch from c09376e to 86c3b7a Compare June 1, 2023 19:15

ryanbas21 temporarily deployed to Preview June 1, 2023 19:15 — with GitHub Actions Inactive

ryanbas21 force-pushed the device-profile-node branch from 86c3b7a to 7485481 Compare June 1, 2023 19:41

ryanbas21 temporarily deployed to Preview June 1, 2023 19:41 — with GitHub Actions Inactive

cerebrl requested changes Jun 1, 2023

View reviewed changes

ryanbas21 force-pushed the device-profile-node branch from 7485481 to f082993 Compare June 1, 2023 21:44

ryanbas21 temporarily deployed to Preview June 1, 2023 21:44 — with GitHub Actions Inactive

ryanbas21 force-pushed the device-profile-node branch from f082993 to 7495dc4 Compare June 1, 2023 21:56

ryanbas21 temporarily deployed to Preview June 1, 2023 21:56 — with GitHub Actions Inactive

ryanbas21 force-pushed the device-profile-node branch from 7495dc4 to 8644839 Compare June 2, 2023 14:53

ryanbas21 temporarily deployed to Preview June 2, 2023 14:53 — with GitHub Actions Inactive

ryanbas21 changed the base branch from main to beta June 2, 2023 15:42

cerebrl reviewed Jun 6, 2023

View reviewed changes

ryanbas21 force-pushed the device-profile-node branch from 8644839 to d6c574b Compare June 7, 2023 22:46

ryanbas21 temporarily deployed to Preview June 7, 2023 22:46 — with GitHub Actions Inactive

cerebrl approved these changes Jun 8, 2023

View reviewed changes

ryanbas21 force-pushed the device-profile-node branch from d6c574b to dc0e45e Compare June 8, 2023 14:57

ryanbas21 temporarily deployed to Preview June 8, 2023 14:57 — with GitHub Actions Inactive

feat(callbacks): device-profile-node addition

a41fa12

add the device profile node to the login widget. can use a device profile node in a page node (with small limitation of timing) or on its own.

ryanbas21 force-pushed the device-profile-node branch from dc0e45e to a41fa12 Compare June 9, 2023 21:45

ryanbas21 temporarily deployed to Preview June 9, 2023 21:45 — with GitHub Actions Inactive

cerebrl added this pull request to the merge queue Jun 12, 2023

Merged via the queue into beta with commit 76dfa5a Jun 12, 2023

cerebrl deleted the device-profile-node branch June 12, 2023 15:40

ryanbas21 added the released on @beta label Jun 12, 2023

ryanbas21 added the released label Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(callbacks): device-profile-node addition #121

feat(callbacks): device-profile-node addition #121

ryanbas21 commented May 11, 2023 •

edited

Loading

ryanbas21 commented May 11, 2023 •

edited

Loading

cerebrl left a comment

cerebrl May 22, 2023

cerebrl May 25, 2023

cerebrl May 25, 2023

cerebrl May 25, 2023

cerebrl May 25, 2023

ryanbas21 May 31, 2023

ryanbas21 May 31, 2023

cerebrl Jun 1, 2023

cerebrl May 25, 2023

cerebrl left a comment

cerebrl Jun 1, 2023

cerebrl Jun 6, 2023

cerebrl left a comment

cerebrl Jun 6, 2023

cerebrl Jun 6, 2023

cerebrl Jun 6, 2023

cerebrl left a comment

cerebrl commented Jun 8, 2023

ryanbas21 commented Jun 8, 2023

ryanbas21 commented Jun 12, 2023

ryanbas21 commented Jul 27, 2023

		// because this is a a function now, i'm using the function defined so
		// the memory is the same and its picked up as such by toEqual.


		import { asyncEvents } from '../../utilities/async-events.js';

		test('Inline widget with login in Canadian French', async ({ page, context }) => {


		### Bug Fixes

		* files: files in package.json means it only updates whats in there ([bdc23ae](https://github.com/forgerock/forgerock-web-login-framework/commit/bdc23ae6c8030b84aea9e92db83a72bee442263a))

feat(callbacks): device-profile-node addition #121

feat(callbacks): device-profile-node addition #121

Conversation

ryanbas21 commented May 11, 2023 • edited Loading

ryanbas21 commented May 11, 2023 • edited Loading

cerebrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cerebrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cerebrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cerebrl left a comment

Choose a reason for hiding this comment

cerebrl commented Jun 8, 2023

ryanbas21 commented Jun 8, 2023

ryanbas21 commented Jun 12, 2023

ryanbas21 commented Jul 27, 2023

ryanbas21 commented May 11, 2023 •

edited

Loading

ryanbas21 commented May 11, 2023 •

edited

Loading