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

Send replies from a journalist to a source #240

Merged
merged 8 commits into from
Feb 8, 2019
Merged

Conversation

heartsucker
Copy link
Contributor

Fixes #16

This PR does implements the following behavior:

  • Journalist can type and sent replies
  • Replies are immediately sent to the API with no queuing / ordering
  • Replies that succeed are stored in the DB
  • Replies that fail are turned red in the UI
    • this is not the final behavior, but is "good enough" for now and can use used as the base for more complex behavior
  • The original ticket discussed using the reply.filename to order messages in the UI, but for now we are naively assuming order sent == order received (i.e., not implementing that behavior). We have additional tickets that will change the underlying DB and this ticket was annoying enough to close as is that I think it's preferable to get this base behavior in now and the add the server-defined ordering in a follow up.

Testing

  • Boot dev container
  • Reply to a source
  • Confirm server receives reply
  • Confirm reply is persisted across client restarts (using --sdc-home CLI arg)
  • Confirm that altering the server code to abort(500) on the reply endpoint cause the bubble to turn red

@heartsucker
Copy link
Contributor Author

Also note that this does not have a migration so we must implement #48 and #203 before the next release.

We need to allow `size` to be NULL becuause when we get a response back
from the API when we post a reply, the `size` field is not present.

Subsequently, we cannot have `size` be a mandatory argument to __init__.
@redshiftzero
Copy link
Contributor

hey @creviera! want to take this through the paces (i.e. run through the test plan here in your dev env and report back on this PR)?

@sssoleileraaa
Copy link
Contributor

sssoleileraaa commented Feb 6, 2019

I was able to confirm @heartsucker's tests:

  • Server receives reply ✔️
    I tested this by sending replies from the client and refreshing the source interface running on the application server
  • --sdc-home CLI arg persists replies ✔️
    I tested this by starting the client via ./run.sh --sdc-home ~/Desktop/client and saw the replies saved in ~/Desktop/client/data and was able to log-out-log-in and still see old conversations
  • Reply bubble is red when server code is 500 ✔️
    I tested this by stopping the application server and sending a reply as a journalist

I took some notes along the way while trying to keep tests within the scope of the reply feature. Sounds like server-defined ordering is happening in a follow up PR, but I documented my experience anyway.

[No Reply Auto-sync]
Debug logs say "Completed message sync" every 5 seconds but unless the journalist manually refreshes the conversation box, new submissions will not appear. Note: Same with reply. The info log says "Updated reply" but the source interface will not show the journalist's reply until they refresh.

[Offline Reply Crash]
The client application crashes when the journalist sends a reply from the journalist interface after signing out:

root:51(excepthook) ERROR: Unrecoverable error
Traceback (most recent call last):
  File "/home/creviera/workspace/freedomofpress/securedrop-client/securedrop_client/gui/widgets.py", line 816, in send_reply
    self.conversation.send_reply(msg)
  File "/freedomofpress/securedrop-client/securedrop_client/gui/widgets.py", line 790, in send_reply
    self.controller.send_reply(self.source.uuid, msg_uuid, message)
  File "/home/creviera/workspace/freedomofpress/securedrop-client/securedrop_client/logic.py", line 670, in send_reply
    self.api.reply_source,
AttributeError: 'NoneType' object has no attribute 'reply_source'

[Re-rendering conversation items]
When the journalist sends a reply it instantly shows up in the conversation box of the client. If a source made a submission before the reply, it will not show up in the conversation box until the journalist manually refreshes. The submission will show up on top of the journalist's reply and then copy the reply below (reordering the conversation text bubbles). The old text bubbles underneath the new ones do not disappear when reordered - you can see layered bubbles.

[Unseen Submission]
Depending on how large the journalist's reply is, the source submission that was sent before the reply might not appear in the conversation box when the journalist refreshes. It'll appear in the middle of the conversation somewhere and that could be outside the scope of the conversation box.

@heartsucker
Copy link
Contributor Author

[Conversation Reordering]

The ordering issue is documented in #226

[No Reply Auto-sync]

This sounds like a pre-existing bug that crept in when we merged #216.

[Unseen Submission]

This sounds like a bug anyway if a source submits many messages. We don't have an --- unread --- notifier.

[Offline Reply Crash]

But this one is a legit bug.

So I propose we fix that one in this PR, and open tickets for the other. @redshiftzero and I talked about keeping this PR tightly scoped for only the most basic functionality. Once this gets in, we can tackle the rest.

Previously, an exception would occur due to self.api being None
when a user is not logged in. Since we're going to address this
soon by, e.g. greying out the reply box when we're not logged in,
let's disallow sending the reply if the journalist
is not logged in. We'll also want to emit the failed signal to
let the developer know what has happened.
@redshiftzero
Copy link
Contributor

hey so there was some discussion in gitter about what we should actually do with respect to not logged in users (i.e. the [Offline Reply Crash] situation noted by @creviera), so i added a guard in f844108 so that we don't actually crash the application and instead the reply just fails to send:

screen shot 2019-02-07 at 5 03 51 pm

we can then implement the behavior we agree on with @ninavizz in #243

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.

thanks @heartsucker and @creviera for the careful review!

@redshiftzero redshiftzero merged commit aa8c9c6 into master Feb 8, 2019
@redshiftzero redshiftzero deleted the send-replies branch February 8, 2019 01:23
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.

Add support for sending replies to sources
3 participants