Script v2: Change priority of CSP related error for verifying manual installations#5707
Script v2: Change priority of CSP related error for verifying manual installations#5707
Conversation
apata
changed the title
apata
force-pushed
the
script-v2/find-script-tag
branch
from
b89c24a to
baa6e35
Compare
| require Logger | ||
|
|
||
| # in this struct, nil means indeterminate | ||
| # In this struct |
There was a problem hiding this comment.
You can define struct fields as atoms, e.g. :selected_installation_type which is a shorter equivalent. Not that you should, just saying.
| expected_domain = "example.com" | ||
| url_to_verify = "https://#{expected_domain}" | ||
|
|
||
| diagnostics = |
There was a problem hiding this comment.
Ah so you went with tests against pure structs. Is there a integration test confirming that this set of field values is possible to achieve?
There was a problem hiding this comment.
Thanks! There's not. You'd be right to point out that this creates false confidence. I tried to manage that by having the rest of the struct randomised , to make sure that exactly these fields determine the result, but reverted it (ccd3d73, why: #5693 (comment)).
Now that I think about, it wouldn't be hard to refactor all these cases to live-view tests, where the verifier request output is mocked... I think we should do that.
There was a problem hiding this comment.
Maybe not as high as live view itself though, but the function doing verification that the LV calls perhaps?
apata
force-pushed
the
script-v2/find-script-tag
branch
from
089035b to
c60f4b0
Compare
| expected_domain = "example.com" | ||
| url_to_verify = "https://#{expected_domain}" | ||
|
|
||
| diagnostics = |
| Routes.site_path(socket, :verification, socket.assigns.site.domain, | ||
| flow: socket.assigns.flow | ||
| flow: socket.assigns.flow, | ||
| installation_type: config.installation_type |
There was a problem hiding this comment.
Nothing wrong with passing the query param, but since the tracker_script_config object gets updated here we can also read the installation_type from that object during the mount phase of the Verification LiveView.
Why? That would fix it for the case where the customer is redirected directly into the verification screen (e.g. from stats_controller.ex) and we don't know the installation method yet.
| cookiesConsentResult, | ||
| error: testPlausibleFunctionError | ||
| } = await testPlausibleFunction({ | ||
| isTrackerInHtml: () => isInHtml(trackerScriptSelector), |
There was a problem hiding this comment.
How about checking this directly in this function without polling, after await testPlausibleFunction is finished? Doesn't have to be in this PR.
There was a problem hiding this comment.
Thanks! I hadn't even considered that, updated in eda2186.
apata
force-pushed
the
script-v2/find-script-tag
branch
from
168bcf1 to
727f13d
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
3 participants
Changes
If the user is verifying a manual installation, CSP error must only be shown after they've actually tried to add the snippet to their site.
Tests
Changelog
Documentation
Dark mode