Fix Basic Captcha Page Functionality #253
Conversation
This commit adds a missing handler for the captcha state. This uses the checkouts handle request captcha method to request a captcha and wait for the token. The handler has also been updated to include error checking.
This commit adds a protocol interceptor to load our captcha html instead of the regular html for other sites. This commit also updates the captcha server middleware stack to better handle loading assets such as images/styling/js while still redirecting everything else to the captcha html page. Issue: #204
Some sites include unsupported size option values, which affects our variant group matching. This commit adds a temporary workaround until the actual issue (#239) is addressed. Additionally, a linter error is fixed, some unnecessary log statements are removed, and another log statement is adjusted to only display relevant information.
This commit addresses some (not all!) captcha related bugs: - Captcha Window Reloads are now handled properly - Redundant Captcha Reset calls are removed - Captcha Windows don't get reset in the middle of harvesting - Attempting to add >5 captcha windows no longer crashes the frontend These are some small bugs that fall under the scope of this branch. Other larger bugs that require separate issues are not fixed in this commit.
pr1sm
added
area:frontend
| @@ -4,6 +4,8 @@ const path = require('path'); | |||
|
|
|||
| const express = require('express'); | |||
| const bodyParser = require('body-parser'); | |||
| // eslint-disable-next-line import/no-extraneous-dependencies | |||
There was a problem hiding this comment.
is there a way to change our linter settings to recognize this? I see this popping up in a lot of files, and I'm not sure if we can just override it globally.
There was a problem hiding this comment.
Yeah there is a way to disable this globally using the .eslintrc.js file. The full configuration is available here.
I don't think we should globally disable the rule since it covers a wide variety of potential errors. Unfortunately, it covers this electron case because we need electron listed as a devDependency instead of a regular dependency (for building to work properly). I noticed there was an option disable this error for all devDependencies, but I'm also a little wary about doing that since electron is a special case (it should be considered a regular dependency as opposed to other devDependencies).
I can mess around with the settings and see if we can change this to a warn, or whitelist 'electron' as a valid case.
There was a problem hiding this comment.
Sounds good! I’ll approve for now 👍🏼
|
merging... |
Changes
Checks
Follow Up Items
Some larger captcha issues have not been addressed in this PR since they are out of the scope. The following table includes the bugs and any issues that have been created to track them.
fixes #204