Add explanatory text for authenticator reset buttons in Journalist/Admin interface

[SaptakS]

Status

Ready for review

Description of Changes

Fixes #1572 .

Changes proposed in this pull request:

• Adds a reusable tooltip sass
• Adds a SASS only approach to show tooltips and avoids large button labels
• Adds necessary functional and page layout tests for the same

Testing

How should the reviewer test this PR?

• Login to the journalist interface.
• Go to the /account/account endpoint
• Hover over the Reset Two-Factor Authentication buttons.
• You should see tooltips similar to this:
  

Checklist

If you made changes to the server application code:

• Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to securedrop-admin:

• Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

• Configuration tests pass

If you made changes to documentation:

• Doc linting (make docs-lint) passed locally

[SaptakS]

I am yet to write the tests. Just wanted to ensure this works. @redshiftzero it's completely in sass. So no JS. 😄

[redshiftzero]

Just tried this out in the dev env - excellent work! These SASS-only tooltips I bet will come in handy on the source interface too.

(By the way, regarding the admin-tests failure here, rebasing on develop will solve it - this is issue #3272)

[SaptakS]

Okay. But I think if possible I should still add functional tests for on hover to check for the tooltip.

[codecov-io]

Codecov Report

Merging #3274 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3274   +/-   ##
========================================
  Coverage    85.76%   85.76%           
========================================
  Files           34       34           
  Lines         2157     2157           
  Branches       238      238           
========================================
  Hits          1850     1850           
  Misses         250      250           
  Partials        57       57

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79854d0...e7b60a0. Read the comment docs.

[heartsucker]

This looks good, but I wonder if we should bump the opacity of the background just a little more to help with accessibility. The text box text collides a bit with the text above it and could make it hard to read on bad screens.

This isn't a blocker, however. Maybe someone with more UX experiences could comment.

[SaptakS]

@heartsucker I can remove or adjust the box-shadow of the tooltip or make the tooltip a little bit above. What do you think? I will check out myself

[heartsucker]

I'm not a frontend person enough to be able to give a good recommendation, but my gut tells me that it could be done in a way that makes the text more readable. That's not so helpful, I know :(

[SaptakS]

@heartsucker updated the opacity of the tooltip to improve accessibility. I didn't notice it before. My apologies.

[heartsucker]

@SaptakS I just pulled your latest changes and confirmed that the change is more readable. However, there were test failures. (Not sure if you can see this link)

test_admin_edit_hotp_secret[ar] - pages-layout.test_journalist.TestJournalistLayout
WebDriverException: Message: waiting for evaluate.js load failed Stacktrace:     at injectAndExecuteScript/r (file:///tmp/tmpdplIUl/extensions/[email protected]/components/driver-component.js:10687)     at fxdriver.Timer.prototype.runWhenTrue/g (file:///tmp/tmpdplIUl/extensions/[email protected]/components/driver-component.js:631)     at fxdriver.Timer.prototype.setTimeout/<.notify (file:///tmp/tmpdplIUl/extensions/[email protected]/components/driver-component.js:625)

The above page-layout test fails on all three locales.

As a note to other reviewers, this does not work on mobile (no hover) and even if it did, the text does not wrap and is chopped off. Related: #1450.

[eloquence]

@SaptakS: Just checking in, do you still have time/interest to work on this, or would you prefer for it to be taken over by someone else?

[SaptakS]

@eloquence I have rebased and fixed the merge conflicts. Working on getting the tests fixed now.

[eloquence]

You're the best, thanks for picking this back up again :)

[zenmonkeykstop]

The rebase is clean and the tooltips look good! Just one thing to fix in the functional tests so that they work for locales other than English...

[SaptakS]

Makes sense.. I will update to use gettext.

[SaptakS]

@zenmonkeykstop Just adding gettext makes the make test command fail since the pagelayout tests don't have an request context and the gettext needs a request context to perform. Is it the same reason why all the other functions with text assertions in pagelayout tests have no gettext?

I am not sure what is the best thing to do in this case because I don't see any similar implementation of sorts. Should I add app context to the pagelayout?

technically inaccurate

[zenmonkeykstop]

This assert will fail if the tests are run against a language other than English. Currently tests that assert the value of a string are only run if the default language is English, and skipped otherwise, like so:

if not hasattr(self, "accept_languages"):                           
                assert explanatory_tooltip_content == tooltip_text

This will allow the page-layout tests to run successfully, while also allowing the functional tests to run against different language settings.

(This probably means we have less functional test coverage for other locales, so it doesn't seem ideal, but that's a problem for another PR.)

[SaptakS]

Makes sense. I will put this in place for now and maybe can check for the locale skipping part in a different PR.

[zenmonkeykstop]

Sorry @SaptakS, my proposed solution wasn't that great! I removed that review and added another more in line with how text asserts in other functional tests are being handled.

[SaptakS]

@zenmonkeykstop I have pushed the changes according to your suggestion. I am not sure what is the CircleCI failure related to. It seems to be related to the source interface, though mine is related to journalist tests.

[zenmonkeykstop]

@SaptakS I don't think the CI failures are related to your code. Looks like a couple of unrelated new CI tests are failing.

[zenmonkeykstop]

Hi @SaptakS , a fix was pushed for the CI issue that is still affecting this PR. If you do one more rebase against develop and resolve the conflicts introduced in securedrop/tests/functional/journalist_navigation_steps.py everything should be good to go!

[kushaldas]

This looks amazing. Thank you. 🌈 Approved.