Scan URL with QR Scanner from Chats #8403
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (6)
|
| @@ -200,6 +200,12 @@ | |||
| (universal-links/handle-url cofx link) | |||
| {:browser/show-browser-selection link})) | |||
|
|
|||
| (fx/defn browser-selection-cancel | |||
There was a problem hiding this comment.
you can use {:events [:browser.ui/browser-selection-cancel]} in that case you don't need to register handler in events ns
There was a problem hiding this comment.
I'd like to keep it the way it is, unless you think it's preferable otherwise
There was a problem hiding this comment.
We are moving to this new method so yes it would be preferable
There was a problem hiding this comment.
I don't completely follow, but is this what you mean? In list_selection.cljs the browse fn will be define as such:
(defn browse [link]
(show {:title (i18n/label :t/browsing-title)
:options [{:label (i18n/label :t/browsing-open-in-status)
:action #(re-frame/dispatch [:browser.ui/open-url link])}
{:label (i18n/label (platform-web-browser))
:action #(.openURL (react/linking) (http/normalize-url link))}]
:cancel-text (i18n/label :t/browsing-cancel)
:on-cancel #(re-frame/dispatch {:events [:browser.ui/browser-selection-cancel]})}))
And this:
(handlers/register-handler-fx
:browser.ui/browser-selection-cancel
(fn [cofx [_ _]]
(browser/browser-selection-cancel cofx)))
is no longer needed in the events ns?
Probably the best option would be to have all QR scanners support all types of URL? |
| [status-im.constants :as constants] | ||
| [status-im.extensions.core :as extensions])) | ||
|
|
||
| (defn- valid-url? [url] |
There was a problem hiding this comment.
What kind of URL do we validate here?
There was a problem hiding this comment.
it uses regx-url defined in status-im.constants
There was a problem hiding this comment.
Probably cleaner to have this logic in a dedicated namespace. Do we have a relevant one already?
There was a problem hiding this comment.
let me know where this helper function should be placed and I will move it. Otherwise i could just have it's implementation within the cond statement itself and not define any helper function.
| {:utils/show-popup {:title (i18n/label :t/unable-to-read-this-code) | ||
| :content (i18n/label :t/use-valid-qr-code {:data data}) | ||
| :on-dismiss #(re-frame/dispatch [:navigate-to-clean :home])}}))) | ||
| (if (valid-url? data) |
There was a problem hiding this comment.
Maybe use some cond instead of the mix of or and if? That would require a predicate to detect universal links.
|
Thanks @krisc! Gave it a test run with 3 cases: General QR code scanning
Scanning from wallet
Scanning from chat
2a and 3b should indeed be invalid as @churik mentioned in point 4 (#7880 (comment)). Thanks for making sure we get this right. The reason they are different stems from the technicalities of keys used inside Status. At the moment your contact code and wallet address are basically the same thing. Therefore yes, you would expect it to be valid to scan either whether you want to chat or transact. However, this is not necessarily the case going forward. The key used for chat and the key used for wallet are theoretically different now and in the near future will actually be different. Requested changesFor 2a, scanning a chat key from wallet, could you replace the copy in the error message with:
For 3b, scanning a wallet address when starting a new chat, could you replace the copy in the error message with:
Also, could you try to have the camera close and the error show up on the New chat screen that holds the input field. This gives more context to the error message. |
98% of end-end tests have passedFailed tests (1)Click to expand
Passed tests (48)Click to expand
|
I agree with Scan QR code from Chat's tab, but not from Add new contact (should just be chat profiles) |
|
@hesterbruikman , thanks for the feedback! I'll get to work on these requested changes. |
|
@hesterbruikman I'm beginning your request for changes for 2a and 3b, but I'm wondering if these changes should even be part of this PR as they don't really address the original issue #5776 although they have to do with the QR Scanner. I'm just wondering if your requests should be their own issue with possibly their own bounty. I don't want to clutter this PR with a bunch of commits that will end up being squashed and undocumented. I'll work on these requests for changes either way :) I'm just not sure how we want to keep this organized from a project management perspective |
|
@krisc I see what you mean. I'll set up seperate bounty in the coming days. There's also some related QR behavior I want to look into. I'll keep you posted once I set up new issue(s)! |
Cool. I seem to be deep into the QR scanner issues lol |
|
@jeluard I tried cleaning it up like this: But the behavior was different, eg., wallet address was interpreted as a URL to open in the browser for some reason. If it's okay, I think I'd prefer to delay cleaning up this area of the code until after @hesterbruikman creates new QR-related issues as was discussed here as I'll probably make more changes to this section the code. I believe the code works as expected right now. But I would like to know why it says |
|
@krisc I'd rather clean it up now, delayed cleanup rarely happen. Seems trivial to have a single cond? |
|
@krisc e2e failure is not related to your PR, this is an infrastructure issue. |
fair enough. I'll try get to this when I get a chance today or tomorrow. |
|
@jeluard EDIT: I now have this: I added a check for deep-links to account for ethereum wallet addresses. With that said, I'm not sure how to manually test for universal-links. Other than that, this works as expected. I still have to change the method of handling events and to move the |
|
Probably out of scope for this PR. I guess we are fine now.
… Le 18 juin 2019 à 22:33, Kris Calabio ***@***.***> a écrit :
@krisc commented on this pull request.
In src/status_im/browser/core.cljs:
> @@ -200,6 +200,12 @@
(universal-links/handle-url cofx link)
{:browser/show-browser-selection link}))
+(fx/defn browser-selection-cancel
I don't completely follow, but is this what you mean? In list_selection.cljs the browse fn will be define as such:
(defn browse [link]
(show {:title (i18n/label :t/browsing-title)
:options [{:label (i18n/label :t/browsing-open-in-status)
:action #(re-frame/dispatch [:browser.ui/open-url link])}
{:label (i18n/label (platform-web-browser))
:action #(.openURL (react/linking) (http/normalize-url link))}]
:cancel-text (i18n/label :t/browsing-cancel)
:on-cancel #(re-frame/dispatch {:events [:browser.ui/browser-selection-cancel]})}))
And this:
(handlers/register-handler-fx
:browser.ui/browser-selection-cancel
(fn [cofx [_ _]]
(browser/browser-selection-cancel cofx)))
is no longer needed in the events ns?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@krisc please, rebase it to current |
|
@krisc thank you for the contribution! 1) After opening URL in native browser can't scan valid URLSteps:
2) White screen after scanning ethereum addressExample of QR: |
|
@churik thanks for the feedback. Issue 2 acts as expected for me, ie., Goes to Send Transaction screen. Not sure why you're getting an error. As for issue 1, should it return to the home screen when changing back to the Status app? |
|
Sorry @krisc for the late feedback. |
|
hey @krisc any updates? thanks |
|
Been busy with hackathons, and another one is starting next week too. But I'll try to see if I can possibly take a look again this week. But if anyone else wants to help me with this, I'll help them get onboard with the issue. |
|
@krisc any chance you can take a look at this? Otherwise I'd suggest we close this PR |
Sorry. Didn't have a chance before the Grow Ethereum hackathon. Seems like it's hackathon season lol. I won't be able to work on Status issues until September. |
|
@krisc No worries, completely understandable! Let's close this PR for now and we can re-open it in the future if things change. Hope that's OK for you. Keep hacking! |
This PR resolves this bounty #5776
Previously was this PR #7880, but issues with nix, it was easier for me to make a new PR.
@churik had some issues in the old PR, but I currently can't recreate them in this PR. Can someone else confirm that these issues are not present?
#7880 (comment)
She also mentioned that Ethereum addresses are also a valid link, and I am not sure what to do about that. I feel like they should indeed be valid. Let me know what I should do about that.
Also just a note, the changes I made to
src/status_im/browser/core.cljsare relevant to #8327 that has not been resolved yet. I wasn't sure if I should have left it in or not, but I just left it in. Let me know if I should remove it.