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

Added source-assignment functionality #1359

Closed
wants to merge 1 commit into from

Conversation

mdrose
Copy link
Contributor

@mdrose mdrose commented Jul 12, 2016

Spent a little time on #1355. This implements a simple version of journalist assignment.

This links a journalist to sources in the database, displays the current value in index.html and gives a reassign link that opens up a dropdown to pick a new journalist. It probably needs some UX improvement to be merged, though.

Using a select dropdown might work if you have a small number of journalists, but if you have a lot, it's probably cumbersome. Maybe a combo box is the way to go? I also thought of making some sort of journalist directory that opens up, but would that be overkill? Not sure how many journalists the average user has. It might also be a good idea to give the option to add names for journalist users. Going only by username could make it less than clear who is being assigned to a source.

This is also implemented without javascript. I'm not really sure what the guidelines are for javascript on document interface. The javascript here seems very minimal. Is this by design? This PR would probably be better with some javascript.

Let me know if you guys have any suggestions.

assignment1

Journalist assignment:
assignment2

@redshiftzero
Copy link
Contributor

This is another really strong PR! Just pulled it down and tested it and everything appears to be working well. In terms of JavaScript, we are primarily concerned with ensuring the source interface is free of JavaScript (since sources should not be using JavaScript), it's less of a concern on the journalist interface - for example, the filtering of sources is currently implemented using JavaScript, so if you wanted to add something there I think it would be fine.

So since this makes modifications to the database, it is another one (like #1422) that we need #1419 implemented for, so we can have production instances moved over to the updated schema. Once that is done, and some unit tests are written - @mdrose let me know if you'd be interested in doing that - I think this should get merged in.

@redshiftzero redshiftzero mentioned this pull request Oct 21, 2016
@psivesely
Copy link
Contributor

Note we "currently allow javascript use in the journalist interface, while gracefully degrading when js is disabled" (see #464 (comment) and #92 (comment)). Journalist assignment should be still possible (through a less pretty or usable interface if need be) when JS is disabled.

Compromise of the server does not mean compromise of past documents, or future documents in the case the source uses PGP. It is a reasonable precaution for a journalist not to use JS on their JW. This security/usability trade-off will eventually be solved by the reading room client, a native application that will query a new (non-HTTP) journalist application server--which we can script to our hearts content because all code will be client-side, and served by signed distribution packages. In the meantime though, this is a nice feature to add.

Note: have yet to review the PR code itself, but will try to get to that this coming week.

@mdrose
Copy link
Contributor Author

mdrose commented Nov 6, 2016

Well, in that case, it seems like this sort of interface is necessary for basic functionality and as a fallback to a future, more usable, Javascript interface. I'll try to get a unit test out for this soon, @redshiftzero.

@psivesely psivesely self-assigned this Nov 8, 2016
@redshiftzero
Copy link
Contributor

Awesome @mdrose! Let me know if you need help or feedback.

@psivesely
Copy link
Contributor

Added "Needs unit/functional tests" label. Going to wait for those to review.

@micahflee
Copy link
Contributor

I haven't actually tested it, but just reading this PR and looking at the screenshots, this looks great to me.

@redshiftzero
Copy link
Contributor

I cherry picked your commit @mdrose, made a tiny change and then added a unit test in a superseding PR #1491 so we can get this in SecureDrop - thanks again for your contributions!! 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants