Css: Delete confirmation with css-only modal #2946

SaptakS · 2018-01-27T21:26:43Z

Status

Work in progress

Description of Changes

Fixes #295.

Changes proposed in this pull request:
According to the discussions in #295 and #2506, I have implemented a CSS-only modal for confirmation of deletion of collections or documents. In this PR, I have replaced the js confirmation by the CSS confirmation modal for

multiple collection deletions
single collection deletion
multiple documents of a single collection deletion

Testing

How should the reviewer test this PR?
Send a message as a whistleblower, log as a journalist mark the message and press Delete. A confirmation modal will appear with options to Cancel or Delete. You can click on Cancel, close icon or anywhere outside the modal to close the modal. On clicking Delete, the selection collection(s)/document(s) will get deleted.

Checklist

If you made changes to the app code:

Unit and functional tests pass on the development VM

SaptakS · 2018-01-27T21:28:36Z

Following is the screenshot of how it looks:

ghost · 2018-01-27T22:09:51Z

It looks awesome 💯 !

ghost · 2018-01-27T22:49:56Z

And it works like a charm: manually tested all I could and did not find a problem. Very goooooood :-)

ghost

This all looks both nice and straightforward. Nicely done !

Please let me know if you need guidance to modify the tests. The failure you see

self = 

    def test_journalist_interface_ui_with_javascript(self):
        self._source_visits_source_homepage()
        self._source_chooses_to_submit_documents()
        self._source_continues_to_submit_page()
        self._source_submits_a_file()
        self._source_logs_out()
        self._journalist_logs_in()
        self._journalist_uses_js_filter_by_sources()
        self._journalist_selects_all_sources_then_selects_none()
        self._journalist_selects_the_first_source()
        self._journalist_uses_js_buttons_to_download_unread()
>       self._journalist_delete_all_javascript()

tests/functional/test_journalist.py:71: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/functional/journalist_navigation_steps.py:688: in _journalist_delete_all_javascript
    self._alert_wait()
tests/functional/functional_test.py:211: in _alert_wait
    'Timed out waiting for confirmation popup.')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = 
method = 
message = 'Timed out waiting for confirmation popup.'

    def until(self, method, message=''):
        """Calls the method provided with the driver as an argument until the \
            return value is not False."""
        screen = None
        stacktrace = None
    
        end_time = time.time() + self._timeout
        while True:
            try:
                value = method(self._driver)
                if value:
                    return value
            except self._ignored_exceptions as exc:
                screen = getattr(exc, 'screen', None)
                stacktrace = getattr(exc, 'stacktrace', None)
            time.sleep(self._poll)
            if time.time() > end_time:
                break
>       raise TimeoutException(message, screen, stacktrace)
E       TimeoutException: Message: Timed out waiting for confirmation popup.

Leads you directly to the files where you can find examples to copy/paste. If this is all too unfamiliar for you I'd be happy to help and provide you with a draft implementation of the tests to get you started.

ghost · 2018-01-27T22:53:34Z

securedrop/sass/journalist.sass

+  background: rgba(0, 0, 0, 0.8)
+  z-index: 99999
+  opacity: 0
+  -webkit-transition: opacity 100ms ease-in


I think we can loose this because we don't support webkit ?

ghost · 2018-01-27T23:05:39Z

securedrop/static/js/journalist.js

-    });
-    if (checked.length > 0) {
-      return confirm(get_string("collection-multi-delete-confirm-string").supplant({ size: checked.length }));
+    var checkboxes = document.querySelectorAll(".submission > [type='checkbox']");


Nit: there is no need to change spacing here and that clarifies this part of the code is left untouched by this PR. Not a blocker for merge at all :-)

Update functional tests for journalist to expect a modal with the cancel and delete button and actions based on it rather than the previous confirmation alert.

Page layout tests now take screenshot of the modal instead of the confirmation message text being saved.

codecov-io · 2018-01-31T20:15:50Z

Codecov Report

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

@@           Coverage Diff            @@
##           develop    #2946   +/-   ##
========================================
  Coverage    89.28%   89.28%           
========================================
  Files           31       31           
  Lines         1810     1810           
  Branches       209      209           
========================================
  Hits          1616     1616           
  Misses         144      144           
  Partials        50       50

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 18aa6ca...6ed9a86. Read the comment docs.

redshiftzero

Great job on this @SaptakS! We just need to wrap the strings used on the buttons in gettext() (see notes inline)

redshiftzero · 2018-01-31T20:44:55Z

securedrop/journalist_templates/col.html

+          <h2>{{ gettext('Delete Confirmation') }}</h2>
+          <p>{{ gettext('Are you sure you want to delete the selected documents?') }}</p>
+          <a href="#close" id="cancel-selected-deletions" title="Cancel" class="btn sd-button">Cancel</a>
+          <button type="submit" name="action" value="confirm_delete" id="delete-selected" class="sd-button danger">Delete</button>


We should make sure each string is wrapped in gettext for translators, so instead of 'Delete', we should use {{ gettext('Delete') }}. This is also true for Cancel.

redshiftzero · 2018-01-31T20:45:12Z

securedrop/journalist_templates/col.html

+        <a href="#close" title="Close" class="close">X</a>
+        <h2>{{ gettext('Delete Confirmation') }}</h2>
+        <p>{{ gettext('Are you sure you want to delete this collection?') }}</p>
+        <a href="#close" id="cancel-collection-deletions" title="Cancel" class="btn sd-button">Cancel</a>


gettext here too

redshiftzero · 2018-01-31T20:45:24Z

securedrop/journalist_templates/index.html

+          <a href="#close" title="Close" class="close">X</a>
+          <h2>{{ gettext('Delete Confirmation') }}</h2>
+          <p>{{ gettext('Are you sure you want to delete the selected collections?') }}</p>
+          <a href="#close" id="cancel-collections-deletions" title="Cancel" class="btn sd-button">Cancel</a>


Last place for gettext

ghost

Impressive job on the tests: their logic is not for the faint of heart 💯

A few comments were added but they are minor: once addressed I'm confident we're good to merge :-)

If you feel inspired (but that's not a blocker for merge), it would be nice to factor the modal HTML blocks into a single file that is included and modified by variables to keep it DRY.

ghost · 2018-02-01T09:50:22Z

securedrop/tests/functional/journalist_navigation_steps.py

-        self._journalist_clicks_delete_selected_javascript()
-        self._alert_dismiss()
+        self._journalist_clicks_delete_selected_link()
+        self._journalist_clicks_delete_selected_cancel_on_modal()


These should be after assert select_count > 0 because this assert verifies there are selected docs to work with. And after self._journalist_clicks_delete_selected_cancel_on_modal() there should be an assert like

assert selected_count == len(self.driver.find_elements_by_name('doc_names_selected'))

to verify no deletion took place.

ghost · 2018-02-01T10:00:09Z

securedrop/static/js/journalist.js

-        return $(this).prop('checked');
-    });
-    if (checked.length > 0) {
-      return confirm(get_string("collection-multi-delete-confirm-string").supplant({ size: checked.length }));


All get_string must also be revomved from the securedrop/journalist_templates/js-strings.html file

Makes sense.

Resolved this.

ghost · 2018-02-01T10:03:36Z

securedrop/sass/journalist.sass

@@ -55,6 +55,58 @@ p.breadcrumbs
  &:dir(rtl)
    text-align: right

+.modal-dialog


What about moving this to modules/_modal.sass or a similar name which would then be included in _base.sass ? Do you envision problems if doing that ?

Will check on this.

redshiftzero

my comments were addressed, 👍 from me

Separated both the html block and the sass code into different files.

SaptakS · 2018-02-01T19:37:33Z

@dachary I have separated out the confirmation modal and tried to write it in a generic way so that even for confirmations other than deletion can use that modal in future. Also separated out the sass code into a module and imported only in journalist.sass because it is being used, for now, only there. Should I move that to _base.sass instead?

ghost

Manually verified to work with the tor browser. 👍 🎉

toast · 2018-02-02T16:18:19Z

Love this. And I like this anchor-based approach @SaptakS

A small thing but perhaps worth mentioning / testing: I noticed in the codepen (from #2506 ) that when I open the modal and and then cancel it; that when I press the back button, the modal pops up again.

ghost · 2018-02-02T18:19:03Z

@toast @SaptakS note that @redshiftzero created a UX category in the forum in case you would like to discuss ambitious plans to improve the SecureDrop UX :-)

SaptakS · 2018-02-03T07:14:37Z

@toast agreed. It also pops up on refresh since I am using anchor Ids and they work that way. Any suggestions from your side what is the best way to deal with it?

It can be dealt with javascript but the whole point is not to use javascript, so.

This page was originally used as a confirmation interstitial when deleting files or messages, but in 1e64f36 (#2946) it was replaced with a CSS-only deletion confirmation on the same page, so users would never hit the separate confirmation page. Remove the tests that were still calling this endpoint, the HTML template and code that was checking for the "confirm_delete" action.

SaptakS requested a review from a user January 27, 2018 21:26

SaptakS requested review from heartsucker and redshiftzero as code owners January 27, 2018 21:26

ghost added feature CSS (Formerly also included SASS) labels Jan 27, 2018

ghost reviewed Jan 27, 2018

View reviewed changes

Css: Delete confirmation with css-only modal

1849529

SaptakS force-pushed the css/delete-confirm branch from 2e85020 to 436c3bb Compare January 31, 2018 10:18

Test: Fix functional test for deletion of collections and documents

1ed2ec3

Update functional tests for journalist to expect a modal with the cancel and delete button and actions based on it rather than the previous confirmation alert.

SaptakS force-pushed the css/delete-confirm branch from 436c3bb to 1ed2ec3 Compare January 31, 2018 10:23

Test: Update page layout tests for deletion of collections and documents

05ad785

Page layout tests now take screenshot of the modal instead of the confirmation message text being saved.

SaptakS force-pushed the css/delete-confirm branch from fcecb9d to 5f0b1e0 Compare January 31, 2018 19:34

redshiftzero suggested changes Jan 31, 2018

View reviewed changes

SaptakS force-pushed the css/delete-confirm branch from 5f0b1e0 to d98b7ee Compare February 1, 2018 05:18

Test: Rename javascript tests to modal for deletion confirmation

51e8472

SaptakS force-pushed the css/delete-confirm branch from d98b7ee to 51e8472 Compare February 1, 2018 05:22

ghost suggested changes Feb 1, 2018

View reviewed changes

redshiftzero previously approved these changes Feb 1, 2018

View reviewed changes

SaptakS added 2 commits February 1, 2018 23:51

Separate out modal into a re-usable block

1e64f36

Separated both the html block and the sass code into different files.

Test: Ensure document and collections are there to be deleted

6ed9a86

SaptakS dismissed redshiftzero’s stale review via 6ed9a86 February 1, 2018 19:33

ghost approved these changes Feb 2, 2018

View reviewed changes

ghost merged commit 4b20ad0 into freedomofpress:develop Feb 2, 2018

redshiftzero added this to the 0.6 milestone Feb 27, 2018

redshiftzero mentioned this pull request Feb 28, 2018

Release SecureDrop 0.6.0 #3061

Closed

22 tasks

legoktm mentioned this pull request Feb 9, 2022

Remove unused confirm_bulk_delete functionality #6261

Merged

2 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Css: Delete confirmation with css-only modal #2946

Css: Delete confirmation with css-only modal #2946

SaptakS commented Jan 27, 2018

SaptakS commented Jan 27, 2018

ghost commented Jan 27, 2018

ghost commented Jan 27, 2018

ghost left a comment

ghost Jan 27, 2018

ghost Jan 27, 2018

SaptakS Jan 28, 2018

codecov-io commented Jan 31, 2018 •

edited

Loading

redshiftzero left a comment

redshiftzero Jan 31, 2018

redshiftzero Jan 31, 2018

redshiftzero Jan 31, 2018

ghost left a comment

ghost Feb 1, 2018

ghost Feb 1, 2018

SaptakS Feb 1, 2018

SaptakS Feb 1, 2018

ghost Feb 1, 2018

SaptakS Feb 1, 2018

redshiftzero left a comment

SaptakS commented Feb 1, 2018

ghost left a comment

toast commented Feb 2, 2018

ghost commented Feb 2, 2018

SaptakS commented Feb 3, 2018 •

edited

Loading

Css: Delete confirmation with css-only modal #2946

Css: Delete confirmation with css-only modal #2946

Conversation

SaptakS commented Jan 27, 2018

Status

Description of Changes

Testing

Checklist

If you made changes to the app code:

SaptakS commented Jan 27, 2018

ghost commented Jan 27, 2018

ghost commented Jan 27, 2018

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 31, 2018 • edited Loading

Codecov Report

redshiftzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redshiftzero left a comment

Choose a reason for hiding this comment

SaptakS commented Feb 1, 2018

ghost left a comment

Choose a reason for hiding this comment

toast commented Feb 2, 2018

ghost commented Feb 2, 2018

SaptakS commented Feb 3, 2018 • edited Loading

codecov-io commented Jan 31, 2018 •

edited

Loading

SaptakS commented Feb 3, 2018 •

edited

Loading