Skip to content
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

Scanning QR Code with a regular provides a bcwallet link on mobile #572

Merged
merged 14 commits into from
Jul 23, 2024
Merged

Conversation

Gavinok
Copy link
Contributor

@Gavinok Gavinok commented Jul 4, 2024

This PR resolves #383

When the QR code is scanned and opened by a regular phone camera the user will now be provided an option to open a BC wallet deeplink.

2024-07-04_14-55-31

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok
Copy link
Contributor Author

Gavinok commented Jul 4, 2024

I also corrected the path to the wallet icon which previously failed to load

@Gavinok Gavinok requested a review from loneil July 4, 2024 21:57
Gavinok and others added 2 commits July 4, 2024 15:14
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@loneil
Copy link
Contributor

loneil commented Jul 5, 2024

Sorry haven't followed along with requirements from this one so maybe already sorted, but wondering about UX for what's supposed to happen to the "camera page"

If I scan with my camera and go to the link I get
image
Then click on the button and it opens the wallet with the proof 👍

So then:

  • On the scanned QR page, the QR doesn't go to pending. Expected due to current deep link setup sending payload to app rather than communications to controller. This is fixed by using the new ?_url style of deep link (which is available to toggle on but needs a couple considerations in the wallet I'm making other issues for). I tried that locally and it goes to pending as expected.
    Known and discussed issue, just noting here so it's described somewhere.

  • I share or decline that proof,, the QR page then actions as I'd expect.

  • But, on the phone, the scanned page (from the screenshot above) is still there without feedback?
    And if I go into the browser and hit the button again it brings up the wallet again but can't handle the proof that's already been actioned.
    So not sure if there's a desired UX for what should happen on the camera-resultant page there.

To really know what happened to the deep link proof it would have to subscribe to the socket/proof pair like the QR code. Or poll for it? Could get messy on the controller side if multiple pages now need to know about the proof callbacks?

Another option is just in-page disable the button and set a status banner/spinner/something just when the button is clicked?

I don't think I see a UX design in the things I have links to for how this page would be intended to behave so maybe not worth handling the above if there's a different UX intended. Could check with Kim next week.

Also think the blue button style would probably want to be matching what's on the other mobile deep link buttons (just the darker blue).

If it's ok to not handle updates on the camera page until later then it could go in as is but might want to get Emiliano's say.

@loneil
Copy link
Contributor

loneil commented Jul 5, 2024

Clip showing user actioning the same deep link by going back to browser:
https://github.com/bcgov/vc-authn-oidc/assets/17445138/ab2f1e5e-4ac7-4411-b6e8-35b22cec4371

@Gavinok
Copy link
Contributor Author

Gavinok commented Jul 5, 2024

I'll reach out to Kim in regards to how we want this to work. As you mention I think more feedback makes sense and some of it isn't that hard though some would require a connection. I'll also split off the styling into a separate file so we can share it across the html files.

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok
Copy link
Contributor Author

Gavinok commented Jul 9, 2024

By default the page will now attempt to redirect mobile users directly to the BC wallet https://0x0.st/XMPT.mp4

@esune
Copy link
Member

esune commented Jul 9, 2024

Sorry I am late to the party here, but have some concerns that may need a chat to clarify:

  • The "you are not using a wallet" page was meant to only indicate the user they are not scanning the QR code with the right tool, and need to try in a different way - that is why there isn't any actionable button there. It is also customizable for each deployment: VC-AuthN is NOT only used by BCGov, even though we are the main developers/maintainers, so we need to (reasonably) think of options that are flexible.
  • If we need to share the logic to subscribe to the websocket and check for an authentication event, we may want to have EVERYTHING on the same page (regular QR code access flow as well as "you didn't use BC wallet"), rather than have to re-implement the same logic in both pages (or import it in both pages if we extract everything to a script). Either way, the structure of the page would have to be the same since selectors should match.
  • Would a "back" button leading to the QR code be a better/easier/simpler solution to this problem? It would prevent having to handle the issue @loneil highlighted.

c.c.: @knguyenBC - so the conversation is not fragmented everywhere

@knguyenBC
Copy link

knguyenBC commented Jul 9, 2024

Given that if the person scanned the QR code with a regular camera and they have BC Wallet installed, the BC Wallet app would still open, I think the "Open with BC Wallet" would not make sense.

I think the more appropriate call-to-action would if the person could download a digital wallet (such as BC Wallet).

Please see this user flow: https://miro.com/app/board/uXjVK0XtU3c=/?moveToWidget=3458764594303727078&cot=14
Here's also a wireframe of a suggested screen: https://www.figma.com/design/8OzWNqLpmh9M9nR0ynTTTh/VC-AuthN?m=dev&node-id=0-1

The wording is a bit more simplified and a new graphic to better visualize a digital wallet.

@esune
Copy link
Member

esune commented Jul 9, 2024

Given that if the person scanned the QR code with a regular camera and they have BC Wallet installed, the BC Wallet app would still open, I think the "Open with BC Wallet" would not make sense.

I think the more appropriate call-to-action would if the person could download a digital wallet (such as BC Wallet).

Please see this user flow: https://miro.com/app/board/uXjVK0XtU3c=/?moveToWidget=3458764594303727078&cot=14 Here's also a wireframe of a suggested screen: https://www.figma.com/design/8OzWNqLpmh9M9nR0ynTTTh/VC-AuthN?m=dev&node-id=0-1

The wording is a bit more simplified and a new graphic to better visualize a digital wallet.

That's what we had in place already - possibly minus the link to download BC Wallet which can be easily added.
To clarify: if the person uses the regular camera app it will NOT open BC Wallet - it will lead them to the above page where they could learn how to install a wallet app.

@knguyenBC
Copy link

If a person is taken to the "Please scan with a digital wallet..." page ,regardless if they were to scan with a regular camera app with BC Wallet installed on their device or without BC Wallet installed on their device, I think its okay to focus more getting the BC Wallet installed.

I think there's a higher chance that a person without BC Wallet installed will try to scan the QR code with a regular camera app than a person with BC Wallet to scan the QR code.

@esune
Copy link
Member

esune commented Jul 10, 2024

If a person is taken to the "Please scan with a digital wallet..." page ,regardless if they were to scan with a regular camera app with BC Wallet installed on their device or without BC Wallet installed on their device, I think its okay to focus more getting the BC Wallet installed.

I think there's a higher chance that a person without BC Wallet installed will try to scan the QR code with a regular camera app than a person with BC Wallet to scan the QR code.

I met with @Gavinok and have to amend my previous statement: I somehow missed the part in his video where it clearly shows that even scanning with the camera app it will trigger the deep link to BC Wallet and continue the authentication flow. I still think adding instructions on how to install BC Wallet (if they get to teh page) and a "back" button to "start over" might be better than trying to tap into the auth flow from there. Thoughts?

@knguyenBC
Copy link

Correct me if I'm wrong, when the person scans the QR code on the webpage, the QR code page doesn't change and the QR code is still scannable so I don't think a back button is necessary. Unless you mean the above webpage that people get to after scanning the QR code with their camera app? I think a back button wouldn't really be useful in that case either as where would the back button go? Unless they had "scanned" the qr code on their mobile device by tapping it? But that's unlikely as they would have seen the mobile view of VC-AuthN.

@loneil
Copy link
Contributor

loneil commented Jul 10, 2024

@knguyenBC do have the image source for the main image on this mockup so @Gavinok can put it on the page

image

@knguyenBC
Copy link

Digital wallet graphic

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok
Copy link
Contributor Author

Gavinok commented Jul 10, 2024

I have changed the howto page the the following
2024-07-10_11-51-44

The deep link will still be tried by default as shown in the previous video and the Download BC Wallet button will link to the corresponding app store depending on the users mobile OS

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@knguyenBC
Copy link

Is it possible to keep the padding/spacing between elements and typography consistent? The figma file should have the font styles. Don't need to follow to a T but maybe use it as a guidance on what should be consistent to each other. Ex. The "Learn more..", "Download BC Wallet", "Don't have a digital wallet", "This QR code..." should be the same font size. Thanks!

@Gavinok
Copy link
Contributor Author

Gavinok commented Jul 10, 2024 via email

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok
Copy link
Contributor Author

Gavinok commented Jul 11, 2024

I have updated the font sizes to be consistent.
2024-07-11_16-04-26

Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some conflicts to address, changes look good otherwise 👍🏻

oidc-controller/api/routers/oidc.py Outdated Show resolved Hide resolved
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok Gavinok requested a review from esune July 12, 2024 21:42
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled this branch locally, and on my machine I'm not getting the central image for the page

Screenshot_20240716-111929

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok
Copy link
Contributor Author

Gavinok commented Jul 16, 2024

@loneil Forgot to add that file. Thanks for testing

@Gavinok Gavinok requested a review from loneil July 16, 2024 23:29
loneil
loneil previously approved these changes Jul 17, 2024
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. Tried using my Pixel 8 and iPad with and without the BC Wallet installed.
Without, the page comes up and I installed the apps from the app store link provided.

With the app installed, the page auto-opens the deep link and actioning that causes the intended effect on the page the QR code was camera'd from.

Note if testing on the current beta android you will run into this bcgov/bc-wallet-mobile#2072
this isn't an issue with this implementation but any existing c_i deep link setups.

Not sure if there's any additional UX considerations (or can be user-tested, sorted out with actual usage as we go forward) for what happens to the page after it auto-loads the deep link? One thing is, if I navigate back, the page is just there telling me to scan with the BC Wallet (probably good? tells me I should be doing it through the wallet next time?)

But if this is the flow we're good with for now all looks good to me.

@esune
Copy link
Member

esune commented Jul 19, 2024

@Gavinok one last conflict to resolve and we can merge this.

Signed-off-by: Lucas ONeil <lucasoneil@gmail.com>
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@esune esune merged commit 648774c into main Jul 23, 2024
6 checks passed
@esune esune deleted the #383 branch July 23, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scanning QR Code with a regular camera app takes user to a dead end
4 participants