feat: add back button to settings/enable-2fa page to allow user to abort #789
Conversation
anGie44
force-pushed
the
anGie44/hushline-761-back-button-2FA
branch
4 times, most recently
from
1fb96aa to
537ab3f
Compare
| print(f"Retrying due to error: {retry_state.outcome.exception()}") | ||
|
|
||
|
|
||
| @retry( |
There was a problem hiding this comment.
Required retrying initial loading of localhost:8080/login because it seems the playwright's browser can spin up before the app (docker compose run) is ready to serve requests.
There was a problem hiding this comment.
If we use pytest fixtures, we can have a liveness check on the app so that it doesn't even return/yield said fixture until it's usable.
| page.goto(f"{BASE_URL}/login", wait_until="domcontentloaded") | ||
| page.get_by_label("Username").fill("test") | ||
| page.get_by_label("Password").fill(user_password) | ||
| page.get_by_role("button", name="Login").click() |
There was a problem hiding this comment.
i tried moving this out into a pytest fixture for the test methods to use before testing the actual button-related changes but couldn't get the tests to synchronously wait for a successful login 🤔 so there's duplication here and test below which isn't great. open to any suggestions here :)
anGie44
force-pushed
the
anGie44/hushline-761-back-button-2FA
branch
from
537ab3f to
a5d3157
Compare
There was a problem hiding this comment.
I think we need to collaborate on a better UI testing strategy since that itself is no small task. There's also so cleanup in the app code like #663 that would make it much easier to test. I would say that likely using a live app + selenium-eque driver would only be for things like JS-only features due to the complexities around setting up an app like that in a way that preserves fixture isolation.
| RUN pip install poetry pytest-playwright | ||
|
|
||
| # Playwright dependencies to run browsers | ||
| RUN playwright install |
There was a problem hiding this comment.
Shouldn't these go after the poetry install so that we get the versions that are pinned in the poetry.lock file? Is there an equivalent playwritght type lockfile
| .PHONY: test | ||
| test: ## Run the test suite | ||
| docker compose run --rm app \ | ||
| poetry run pytest --cov hushline --cov-report term --cov-report html -vv $(PYTEST_ADDOPTS) $(TESTS) | ||
|
|
||
| .PHONY: test/ui | ||
| test/ui: # Only run the ui tests |
There was a problem hiding this comment.
I think we way we'll want to split this is either using some kind of pytest.mark or possibly a wholly separate directory like ui-test since we'd need a separate set of fixtures for a live app, though there might be some clean way to make them overlap.
| @@ -29,6 +29,8 @@ stripe = "^10.9.0" | |||
| types-requests = "^2.32.0.20240712" | |||
| types-setuptools = "^71.1.0.20240813" | |||
| python = "^3.11" | |||
| pytest-playwright = "^0.6.2" | |||
| from playwright.sync_api import Page, expect | ||
| from tenacity import RetryCallState, retry, stop_after_attempt, wait_random_exponential | ||
|
|
||
| BASE_URL = "http://127.0.0.1:8080" |
There was a problem hiding this comment.
This URL (or rather, port) should probably come via a fixture of some sort using a trick like this:
I'm guessing the UI tests require an already running app that's started in a second terminal or so, yeah?
| print(f"Retrying due to error: {retry_state.outcome.exception()}") | ||
|
|
||
|
|
||
| @retry( |
There was a problem hiding this comment.
If we use pytest fixtures, we can have a liveness check on the app so that it doesn't even return/yield said fixture until it's usable.
Closes #761
What and How 🛠️
This PR introduces a back button in the
settings/enable-2faview. The button is added via the<a>element in the page's template file.Why 🤷
The new link serves as a an escape hatch, allowing users to return to the authentication settings page.
Testing 🧪
To validate the existence and usage of the
<a>element representing the back button, I've leveraged Playwright's browser testing. The test cases live in auisubdirectory so they can be run independently with a new target, namelymake test/ui, or alongside all the the tests viamake test.Locally, you can pull this branch and run
make test/uito validate.Anything else 💭
I chose Playwright here instead of Selenium because it seems to spin up faster and perhaps less overhead -- did require installing some dependencies (see
Dockerfile.dev). Open to discussing this choice more since sets a precedent for e2e browser testing