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

Handle multiple instances of source /generate #5048

Conversation

DrGFreeman
Copy link
Contributor

Status

Work in progress

Description of Changes

Fixes #4458.

Changes proposed in this pull request:

When the source opens more than one browser tabs on /generate before clicking on SUBMIT DOCUMENTS:

  • Flashes a message promting the source to verify their codename in /lookup after they click on SUBMIT DOCUMENTS in a fist instance of /generate.
  • Redirects the source to /lookup when they click on SUBMIT DOCUMENTS in a second instance of /generate and displays a flash message prompting the source to verify their codename.
  • Makes the codename in /lookup visible by default.

This eliminates the 500 server error reported in #4458 and reduces the potential of codename confusion following the generation of multiple codenames in different browser tabs or windows.

The behavior when only one instance of /generate is opened remains unchanged.

Testing

How should the reviewer test this PR?
Write out any special testing steps here.

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make 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:

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

If you added or updated a code dependency:

Choose one of the following:

  • I have performed a diff review and pasted the contents to the packaging wiki
  • I would like someone else to do the diff review

When the source opens more than one browser tabs on /generate before clicking
on SUBMIT DOCUMENTS:
-Flashes a message promting the source to verify their codename in /lookup
 after they click on SUBMIT DOCUMENTS in a fist instance of /generate.
-Redirects the source to /lookup when they click on SUBMIT DOCUMENTS in a
 second instance of /generate and displays a flash message prompting the
 source to verify their codename.
-Makes the codename in /lookup visible by default.

This eliminates the 500 server error reported in freedomofpress#4458 and reduces the
potential of codename confusion following the generation of multiple
codenames in different browser tabs or windows.

The behavior when only one instance of /generate is opened remains unchanged.
@DrGFreeman
Copy link
Contributor Author

DrGFreeman commented Dec 1, 2019

This PR follows the discussions in #4996.

It is a work in progress with placeholder messages for which I would like to get feedback from the team before going forward in updating the tests and implementing new ones.

Here is a demonstration of the proposed behavior:

  1. Source opens /generate in tab A. Codename X is in session.
    image
  2. Source opens /generate in tab B. Codename Y is in session.
    image
  3. Source returns to tab A and clicks "SUBMIT DOCUMENTS". Codename Y is used to register the source. Changes relative to the current behavior: A flash message is added and the codename is made visible:
    image
  4. Source returns to tab B and clicks "SUBMIT DOCUMENTS". Changes relative to current behavior: No server error (500). The source is redirected to /lookup, a flash message is displayed and the codename is made visible:
    image

If, alternatively, the source clicked "SUBMIT DOCUMENTS" in tab B first and then in tab A, the result would be similar except the flashed messages would be interverted.

There is no change in the interface behavior when only one /generate page was opened (i.e. no flash message, codename hidden).

Also, an alternative to making the codename visible would be to add a link in the flash message similar to when the source makes their first submission:
image

@DrGFreeman DrGFreeman changed the title Handle multiple instances of source /generate (#4458) Handle multiple instances of source /generate Dec 1, 2019
Update the two existing tests to reflect the source interface behavior
modification introduced by c67db8e.
In both cases, the source should be redirected to the /lookup page
with a relevant flash message.
@zenmonkeykstop
Copy link
Contributor

From a UX perspective, this LGTM. Tested as follows:

2x get started

  • ra make dev
  • opened 2 tabs pointing at localhost:8080 and clicking through Get Started on both. Two different codenames generated
  • clicked thru Submit Documents on first tab. First tab's codename displayed on reminder. Successful submit.
  • clicked thru Submit Documents on second tab. Flash message displayed and first tab's codename displayed on reminder. Successful submit.
  • logged out on first tab, got returned to Source Interface homepage
  • tried to submit on second tab, got returned to SI homepage with expired session flash message (that might need to be updated to have more generic language as it says the session timed out due to inactivity).

get started, login in separate tabs

  • opened 2 tabs pointing at localhost:8080 and clicked Get Started on one, Log in on the other.
  • logged in on 1st tab using existing codename - login successful
  • clicked Submit Documents on 2nd tab. Flash message displayed and first tab's codename displayed on reminder.

JI check

  • checked Journalist interface, both submissions are present under a single codename corresponding with the SI codename (tested using replies).

Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

LGTM based on manual testing. Will hold off on merge pending a more exhaustive code review (due to implications of session changes).

@DrGFreeman
Copy link
Contributor Author

Thanks for the comments @zenmonkeykstop.

I marked the PR as WIP to make sure it was going in the right direction following the comments in #4996.

I still want to add functional tests before it is merged and would also appreciate feedback on the wording of the flash messages as I am not native English speaker and there may be better ways to phrase these.

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.

the approach here looks good, if you add functional tests prior to merge that would be excellent

# Issue 2386: don't log in on duplicates
del session['codename']

# Issue 4361: Delete 'logged_in' if it's in the session
Copy link
Contributor

Choose a reason for hiding this comment

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

confirming that #4391 was a fix required when we were deleting session['codename'] in this view function and so is good to remove now

securedrop/source_app/main.py Outdated Show resolved Hide resolved
@zenmonkeykstop
Copy link
Contributor

The flash messages look good to me with the exception of one period :). With functional test updates, this should be good to go.

@eloquence
Copy link
Member

eloquence commented Dec 5, 2019

I think we're definitely closer to a resolution here, thanks for all your hard work on this @DrGFreeman. If some version of this lands, I'd like to give @ninavizz a chance to help with the final UI messaging at minimum. She suggested in the other PR that we might want to display the codename inline in the message; that could potentially be handled as a separate issue.

I do think we should talk a bit more whether this is the best approach, though. I'm getting a "multiple codenames" message on the subsequent page if I just reload the /generate page in the original tab (which the user may wish to do to generate a new codename). That's IMO a UX regression -- I suspect reloading the original tab is going to be more common than having more than one tab open. And it's confusing, because it warns the user about multiple tabs when they only have one open.

@DrGFreeman
Copy link
Contributor Author

Hi, thanks to all for the comments.

I'm getting a "multiple codenames" message on the subsequent page if I just reload the /generate page in the original tab (which the user may wish to do to generate a new codename). That's IMO a UX regression.

@eloquence, I agree with you, this is not desirable. I should have thought of checking this in the first place.

I've been thinking about how to solve this issue elegantly for a while now and I think the fundamental issue preventing this is that we currently cannot uniquely identify each browser tab in which a codename is generated. As per my previous comment, if we could do so, it would be possible to ensure the codename displayed in the tab from which the user clicks "SUBMIT DOCUMENTS" is the one used for the login and thus avoid having to use the flash message altogether. The only message required would be the "already logged in, please check your codename" message when the user clicks "SUBMIT DOCUMENTS" in the second tab. If someone experienced could comment on the feasibility of my proposal and potentially give some high level direction of how to make it work, I'd be glad to take the time and try to implement it.

I'd like to give @ninavizz a chance to help with the final UI messaging at minimum. She suggested in the other PR that we might want to display the codename inline in the message;

Absolutely! That's why I set this PR to WIP.

I see three options with respect to displaying the codename to the user:

  1. By making it visible at the bottom (what I have implemented here),
  2. As a link in the flash message (as suggested at the bottom of my first comment on this PR),
  3. Directly in the flash message.

I think that 1 and 2 have the advantage of allowing the source to hide back the codename while proceeding with their submission which cannot be done if it is included in the flash message.

@zenmonkeykstop
Copy link
Contributor

Another option might be to move the regular "remember, your codename is.. " box up to the top, so that it's always front and center for new logins. Then if folks are getting the multiple tabs flash, the thing they need to check is just below it.

@DrGFreeman
Copy link
Contributor Author

Closing in favor of #5075 which resolves the refresh issue identified by @eloquence above and in my opinion offers a much smoother user experience (source registered with the codename displayed in the tab from which they proceed to submit documents and only one flash message when the source is already logged-in).

@DrGFreeman DrGFreeman closed this Dec 14, 2019
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.

Duplicate Source Interface session triggers server error
4 participants