Inline conversation updates. Fixes #473.

[ntoll]

Description

Fixes #473.
Fixes #635.
Fixes #691.

This is proof of concept ensures we're not deleting and recreating messages in a conversation each time a new message is added to the conversation or an update occurs.

It also handles cases where the messages are re-ordered after an update, and makes it possible to inline-update further attributes of messages as they change through time.

It was also an opportunity for me to deep-dive back into the guts of the application and figure out how it all fits together again..! 🔬 👀 🐍 What you see is my stab at a simple and flexible solution.

The comments / annotations in the code should explain what's going on. Please take a look and, as always, I welcome ideas, constructive criticism and other feedback. Assuming, via discussion of the PR, we settle on the approach, I can pick this up again on Monday (my next FPF day) and make it into a "proper" PR with tests and other essentials.

Thanks in advance for your contributions..! 👍

Test Plan

To be done.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

• I have tested these changes in the appropriate Qubes environment
• I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
• These changes should not need testing in Qubes

[ntoll]

Ok, so I've been looking into this.

@creviera I may be missing something here, but AFAICT it already works in terms of MessageWidget instances and update signals.

In the add_message method we see this:

def add_message(self, message: Message, index) -> None:
        """
        Add a message from the source.
        """
        if message.content is not None:
            content = message.content
        else:
            content = '<Message not yet available>'

        conversation_item = MessageWidget(message.uuid, content, self.controller.message_ready, index)
        self.conversation_layout.insertWidget(index, conversation_item, alignment=Qt.AlignLeft)
        self.current_messages[message.uuid] = conversation_item

Note the line three from the bottom where the MessageWidget is connected to the message_ready signal from the controller layer. As I understand it (and looking at the _update_text method that ultimately handles this signal, if the message is updated then the text will automatically change as the code currently stands.

I'm probably misunderstanding what you're asking for here, but when I pretend to be a source via the local version of SD website, I don't even appear to have the opportunity to update or change the content of the message I (as a source) have sent. I.e. the content of MessageWidget instances should never change given the way things currently stand.

I have a feeling I'm getting the wrong end of the stick here. ;-)

[ntoll]

OK... latest commit fixes #61 and #685 since this work needed the inline updates found in this ticket to work.

Feedback, constructive criticism and ideas most welcome. 👍

[sssoleileraaa]

Also fixes #635

[sssoleileraaa]

@creviera I may be missing something here, but AFAICT it already works in terms of MessageWidget instances and update signals.

Ah you are right. We already connect the message_ready signal for all speech bubbles which updates the text after a message is successfully downloaded and decrypted:

securedrop-client/securedrop_client/gui/widgets.py

Lines 1572 to 1581 in 15314ae

	update_signal.connect(self._update_text)
	@pyqtSlot(str, str)
	def _update_text(self, message_id: str, text: str) -> None:
	"""
	Conditionally update this SpeechBubble's text if and only if the message_id of the emitted
	signal matches the message_id of this speech bubble.
	"""
	if message_id == self.message_id:
	self.message.setText(text)

So you don't need to worry about updating the MessageWidget since it is a SpeechBubble widget which handles the signal by updating the text

[redshiftzero]

just read through this diff, this approach so far makes sense to me!

[redshiftzero]

ah, this is a nice way to handle the scrolling behavior

[ntoll]

OK folks... this is ready to review now that all the tests pass. ;-)

Something to bring to your attention:

• on_reply_sent used to immediately add the reply to the conversation. This no longer happens because it was creating duplicate entries in the conversation view for the reply item. However, there needs to be some indication that the reply is in flight. I can easily add something to this PR (a spinner or some other such signal as we may agree/discuss with @ninavizz). Alternatively, this could be a new issue.

Hurrah! 🎉 💬

[sssoleileraaa]

• on_reply_sent used to immediately add the reply to the conversation. This no longer happens because it was creating duplicate entries in the conversation view for the reply item. However, there needs to be some indication that the reply is in flight. I can easily add something to this PR (a spinner or some other such signal as we may agree/discuss with @ninavizz). Alternatively, this could be a new issue.

The current implementation of pending replies saves them to the draftreplies table and then removes them from there when they have been successfully sent. In the GUI these pending replies show up as more transparent as you see here in the design: https://app.zeplin.io/project/5c807ea562f734bd2756b243/screen/5d5b183a152c905223e5fab0

See how we add a draft reply with a pending status to the local db before creating a job and sending it off to the server:

securedrop-client/securedrop_client/logic.py

Lines 787 to 799 in f391abe

	reply_status = self.session.query(db.ReplySendStatus).filter_by(
	name=db.ReplySendStatusCodes.PENDING.value).one()
	draft_reply = db.DraftReply(
	uuid=reply_uuid,
	timestamp=datetime.utcnow(),
	source_id=source.id,
	journalist_id=self.api.token_journalist_uuid,
	file_counter=source.interaction_count,
	content=message,
	send_status_id=reply_status.id,
	)
	self.session.add(draft_reply)
	self.session.commit()

And here you'll see how we delete the draft reply and add a non-draft reply within the SendReplyJob:

securedrop-client/securedrop_client/api_jobs/uploads.py

Lines 59 to 65 in 6be344c

	update_draft_replies(session, source.id, draft_timestamp,
	draft_file_counter, new_file_counter)
	# Delete draft, add reply to replies table.
	session.add(reply_db_object)
	session.delete(draft_reply_db_object)
	session.commit()

Hopefully this points you to the code and design docs you need to see why we have this feature and how it should work.

[ntoll]

@creviera your comments about on_reply_sent have been addressed. Many many thanks for the pointers -- I'd missed those aspects of the code and they were really useful 🎉 Please check. Thanks..! :-)

[ntoll]

Hmmm... not sure why the tests failed. They work for me locally. Investigating.

[ntoll]

OK... looked into the CI fail. Not sure what's going on since the tests are supposed to be isolated from each other and the test is exactly the same as test_update_conversation_removes_draft_items beyond the point of failure. I.e. exactly the same piece of code passed in a previous test. My guess is leaky state somewhere in the tests, but it always passes for me and I don't have permissions to re-run and SSH into the instance to figure out what's going on. HELP..! :-)

[sssoleileraaa]

🎉 I see pending replies show up now! I haven't looked at why your test is failing yet, but it looks like the ConversationView widgets are missing their spacing and sometimes overlap when the GUI is updated, see:

I wonder if this is a layout spacing issue or perhaps the index logic that determines where we need to start redrawing widgets has a bug in it. I can take a closer look later today.

[ntoll]

@creviera hmm... I didn't encounter the spacing issue while testing (highly likely to be my oversight). Feel free to bat it back in my court -- happy to fix up my own mistaikes. ;-)

[eloquence]

@ntoll Per Allie's comment above, I've updated the PR description for this PR to also resolve #635 and removed that issue from the board; please double-check that this is indeed correct :)

[ntoll]

@eloquence that is, indeed, the case (wrt #635)

[ntoll]

@creviera OK... I've been able to reproduce and fix all the bugs you mention except the final one (which I've not been able to reproduce, despite no end of redrawing, rescaling, maximising, minimizing, sending of messages and so on). The actual fix itself was very simple and addressed a bug where the widget was drawn over or mistakenly deleted if in a failed-to-send state. I've also added a test to ensure this case is explicitly checked.

Also, thanks for sharing that really cute trick for making SD server randomly fail. That helped a LOT. 👍

[ntoll]

Also, trying to figure out how to properly test with a ReplySendStatus took some time to work out. (My initial strategy just to mock it away caused all sorts of problems, hence the new test/factory.py additions).

[sssoleileraaa]

@creviera OK... I've been able to reproduce and fix all the bugs you mention except the final one (which I've not been able to reproduce, despite no end of redrawing, rescaling, maximising, minimizing, sending of messages and so on).

amazing! will test and verify

[sssoleileraaa]

I tested all the following bugs, which no longer occur:

• Bug: Disappearing replies
• Bug: Failed reply bar not always the correct color
• Bug: Failed status being wrapped to next line
• Bug: Filling width of ConversationView with multiple speech bubbles

Nice work!

[sssoleileraaa]

Note: once you rebase, we can merge

[ntoll]

Rebased. 👍

[sssoleileraaa]

I don't have an STR yet, but I'm seeing a new issue where existing speech bubbles are getting added to the conversation view more than once. It looks like whenever I send a reply that fails, then existing successful messages and replies are added again to the conversation. This might happen only when resizing the window. I will try to create an STR to repro but here you can see that the "This is a test submission without markup!" and "This is a test submission with markup..." have been copied multiple times:

[sssoleileraaa]

I am unable to repro the repeating messages and replies. Since it has only happened once for me, I will create an issue in case someone else runs into this.

[sssoleileraaa]

looks like this needs another rebase

[ntoll]

Rebased.

[sssoleileraaa]

looks like you need to rebase onto master again, the conflict is:

--- a/securedrop_client/gui/widgets.py
+++ b/securedrop_client/gui/widgets.py
@@@ -2483,10 -2436,16 +2517,21 @@@ class ConversationView(QWidget)
          """
          Add a message from the source.
          """
++<<<<<<< HEAD
 +        conversation_item = MessageWidget(message.uuid, str(message), self.controller.message_ready)
 +        self.conversation_layout.addWidget(conversation_item, alignment=Qt.AlignLeft)
++=======
+         if message.content is not None:
+             content = message.content
+         else:
+             content = '<Message not yet available>'
  
-     def add_reply(self, reply: Union[DraftReply, Reply]) -> None:
+         conversation_item = MessageWidget(message.uuid, content, self.controller.message_ready, index)
+         self.conversation_layout.insertWidget(index, conversation_item, alignment=Qt.AlignLeft)
+         self.current_messages[message.uuid] = conversation_item
++>>>>>>> A working PoC which requires feedback and tests. Fixes #473.
+ 
+     def add_reply(self, reply: Union[DraftReply, Reply], index) -> None:
          """
          Add a reply from a journalist to the source.
          """

[ntoll]

@creviera OK... some careful git unpicking and a rebase later... I've had to force push. Please check.

[sssoleileraaa]

all pr comments were addressed and bugs fixed. time to merge!