Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove jquery #4211

Merged
merged 2 commits into from
Mar 5, 2019
Merged

Remove jquery #4211

merged 2 commits into from
Mar 5, 2019

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Feb 27, 2019

Status

Ready for review

Description of Changes

Removes jQuery. This updates the work done by @kaizensoze for #1233 to account for changes made to the UI since.

  • On journalist index page, make select all/none apply only to visible sources, not those hidden by an active filter. Add test for this.
  • Handle confirmation for both types of MFA token reset.
  • Close Tor slider popup with an X, as done elsewhere.
  • Fix JS handling of "download unread" button.
  • Fix some duplicate id attributes.
  • Add flexbox SASS module.
  • Improve detection of Tor browsers.

Fixes #1233.

Testing

In the source interface, JavaScript is used to suggest Tor Browser with the security slider set to safest.

  • Check that when visiting a dev instance with a regular desktop browser, you get the purple banner suggesting Tor Browser, that the instructions link still works, and that clicking the X icon to close the warning fades it out.
  • (Optional) For bonus points, check Orfox from a phone or emulator, and verify that you're urged to use a desktop browser (I have tested this).
  • Finally, visit the site with Tor Browser, with the security slider set to standard, to verify that you're encouraged to move it to safest. The instructions link should result in a popup which you can likewise close with the X icon (this is a UX change; previously this popup would be closed by clicking outside of it).

This change also replaces jQuery usage in the journalist interface.

  • Log in, and on the source list, confirm that you can select all then select none, with the checkboxes updating accordingly.
  • Type part of the first source's codename in the filter box. Click to select all, then clear the filter box and verify that only the first source was selected.
  • Click to select all, then use the filter to show just the first source and click to select none. Clear the filter and confirm that only the first source was deselected.
  • Find a source with unread messages, and in that source's row, click the button to download the messages. The button should disappear, and the row should now have the "read" styling.
  • Click on a source to navigate to their collection page. Verify that the buttons to select all, unread, or none work as expected.

Deployment

This included an AppArmor change to permit Apache to read an icon that wasn't used before. Verify that when visiting the source interface with Tor Browser with the security slider set to standard, that when you click to see instructions, there is a black X icon in the upper right corner of the popup. If you get a broken link, the AppArmor profile may not have been updated correctly.

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 the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

Joe Gallo and others added 2 commits February 27, 2019 08:33
Account for changes made to the UI since the work was started.

- On journalist index page, make select all/none apply only to visible
  sources, not those hidden by an active filter. Add test for this.
- Handle confirmation for both types of MFA token reset.
- Close Tor slider popup with an X, as done elsewhere.
- Fix JS handling of "download unread" button.
- Fix some duplicate id attributes.
- Add flexbox SASS module.
- Improve detection of Tor browsers.
@rmol rmol marked this pull request as ready for review February 27, 2019 20:28
Copy link
Contributor

@heartsucker heartsucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that the test plan passes (except testing on mobile). I am, however, not a JS expert, so someone else might want to look at the code and see if there's any errors. I mean, I did look at it, but it might need another pass.

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great! verified manually that js functionality on both interfaces degrades gracefully with security slider set to highest

👍 from me

@redshiftzero
Copy link
Contributor

@creviera want to also review this one before we merge?

@sssoleileraaa
Copy link
Contributor

heyyy tests pass for me! I didn't check the Orfox one.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the source interface, JavaScript is used to suggest Tor Browser with the security slider set to safest.

  • Check that when visiting a dev instance with a regular desktop browser, you get the purple banner suggesting Tor Browser, that the instructions link still works, and that clicking the X icon to close the warning fades it out.
  • (Optional) For bonus points, check Orfox from a phone or emulator, and verify that you're urged to use a desktop browser. Tested on both Onion Browser on iOS and on Tor Browser on Android.
  • Finally, visit the site with Tor Browser, with the security slider set to standard, to verify that you're encouraged to move it to safest. The instructions link should result in a popup which you can likewise close with the X icon (this is a UX change; previously this popup would be closed by clicking outside of it).

This change also replaces jQuery usage in the journalist interface.

  • Log in, and on the source list, confirm that you can select all then select none, with the checkboxes updating accordingly.
  • Type part of the first source's codename in the filter box. Click to select all, then clear the filter box and verify that only the first source was selected.
  • Click to select all, then use the filter to show just the first source and click to select none. Clear the filter and confirm that only the first source was deselected.
  • Find a source with unread messages, and in that source's row, click the button to download the messages. The button should disappear, and the row should now have the "read" styling.
  • Click on a source to navigate to their collection page. Verify that the buttons to select all, unread, or none work as expected.

Approved. 👍

@kushaldas kushaldas merged commit 0695249 into freedomofpress:develop Mar 5, 2019
@redshiftzero redshiftzero added this to the 0.13.0 milestone May 14, 2019
@rmol rmol mentioned this pull request May 22, 2019
17 tasks
@rmol rmol mentioned this pull request Jun 17, 2019
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite static/js/source.js removing jquery requirement for source interface
5 participants