Redirect source to /lookup if visiting /generate and already logged in #1080
Conversation
|
Thanks for submitting this, @pwplus! A nice corollary to this change would be to add an explicit "logout" button to the Source Interface. See #682 for context and conversation around this proposal. This PR is a reasonable fix for the issue discussed in #682 (comment). |
|
@pwplus Before I merge this, would you consider adding a test? The right place to put it is The application unit tests are kind of fiddly and annoying right now, so if you have any questions or run into any problems, just let me know. Refactoring them is one of my goals for the next major release. |
|
@garrettr, I would be more than happy to write both the tests and the corollary. I have already written the functionality of the logout button. UX is not something I have worked with often and especially not with CSS, so that part is taking a little longer. There is a design decision that I'm unsure of: I routed a new function that logs you out to /logout. It uses a flash notification letting them know they were logged out and thanking them. Should it redirect to the login page or the index? Currently I'm redirecting to the index. After this done, I would be interested in assisting you in refactoring the tests. The only catch is that I have a final round of interviews with a large software company on Monday. Assuming I get the job, I would need to verify that I'm not breaking any contract while working on this project. I'll definitely get the changes I'm working on before they have a chance to give me an offer, so this set of changes will definitely be fine. Finally, as a matter of procedure, would you rather the corollary and tests be pushed as a single commit with this pull request or as separate commits? |
|
Sorry I'm not getting to this quicker; I'm preparing for an interview on Monday. I decided to go ahead to push the logout functionality as a separate commit since I'm taking so long. I figured you can at least see it this way. I looked over the tests earlier today to get a feel for them. I'll try to write the test and add the button later tonight. After all the changes have been made, I will squash all of the smaller commits into one large one. As for the log out button, do you think that should go in securedrop/source_templates/base.html or into just the lookup page? It seems that if I put it in base.html that there is no way to get it inside of the thin grey box. I don't however know if it would be wise to only have the logout button on the lookup page. I've never had much of an eye for aesthetically pleasing user interface decisions, so I'll let you make this call. |
That sounds fine to me. We can always change it later if we decide it's more ergonomic to go back to the login page. |
The lookup page is the only page that a user must be logged in to access, so it would make sense to only put it there. I guess they could also go to the index page and you might want the logout button to be there too, in which case you could put it in If you have the logout button setup and working, but don't like the way it looks, feel free to push it and I can refine the styles for you. |
No worries, it's open source. We're really stoked that you're contributing in the first place! Good luck with your interview if it hasn't already happened 🤘 |
|
I think I added a decent looking logout button. Here is a screen shot with all UX changes visible. I'll squash all these commits into a single commit and write a proper commit log in a few hours. Is there anything else you would like done? |
Fixes #1080 We have many test strings for testing the client text formatting. One can enable test data by: `CLIENT_TEST_DATA=1 NUM_SOURCES=20 make dev`
Fixes #1080 We have many test strings for testing the client text formatting.
Fixes #1080 We have many test strings for testing the client text formatting.
One can get all the client source strings by using the `NUM_SOURCES=ALL make dev` command. The `make dev` command will create 2 sources as usual. One can also assign the number of sources (say 10) by `NUM_SOURCES=10 make dev` command. Fixes #1080 Special strings sourced from: https://github.com/freedomofpress/securedrop-client/wiki/Message-Test-Data
If a source that is already logged in ends back up on the /generate page, it redirects them to /lookup rather than logging them out and generating a new codename. This was listed as possibly better behavior in the comments at the top of the generate function in source.py.