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.

Sign up for GitHub

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

Remove ConversationView.add_reply_from_reply_box #834

Closed
wants to merge 2 commits into from
Closed

Remove ConversationView.add_reply_from_reply_box #834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a4e672
Simplify how a reply is added from the replybox
Mar 10, 2022
3aeef6e
Remove old way of checking deletion timestamp
Mar 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 16 additions & 53 deletions securedrop_client/gui/widgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1329,11 +1329,6 @@ def set_snippet(
if self.deleting or self.deleting_conversation:
return

# If the sync started before the deletion finished, then the sync is stale and we do
# not want to update the source widget.
if self.sync_started_timestamp < self.deletion_scheduled_timestamp:
return

# If the source collection is empty yet the interaction_count is greater than zero, then we
# known that the conversation has been deleted.
if not self.source.server_collection:
Expand Down Expand Up @@ -2944,18 +2939,15 @@ def _on_conversation_deletion_successful(self, source_uuid: str, timestamp: date
logger.debug(f"Could not update ConversationView: {e}")

def update_deletion_markers(self) -> None:
try:
if self.source.collection:
self.scroll.show()
if self.source.collection[0].file_counter > 1:
self.deleted_conversation_marker.hide()
self.deleted_conversation_items_marker.show()
elif self.source.interaction_count > 0:
self.scroll.hide()
self.deleted_conversation_items_marker.hide()
self.deleted_conversation_marker.show()
except sqlalchemy.exc.InvalidRequestError as e:
logger.debug(f"Could not Update deletion markers in the ConversationView: {e}")
if self.source.collection:
self.scroll.show()
if self.source.collection[0].file_counter > 1:
self.deleted_conversation_marker.hide()
self.deleted_conversation_items_marker.show()
elif self.source.interaction_count > 0:
self.scroll.hide()
self.deleted_conversation_items_marker.hide()
self.deleted_conversation_marker.show()

def update_conversation(self, collection: list) -> None:
"""
Expand All @@ -2976,11 +2968,6 @@ def update_conversation(self, collection: list) -> None:
passed into this method in case of a mismatch between where the widget
has been and now is in terms of its index in the conversation.
"""
# If the sync started before the deletion finished, then the sync is stale and we do
# not want to update the conversation.
if self.sync_started_timestamp < self.deletion_scheduled_timestamp:
return

self.controller.session.refresh(self.source)

# Keep a temporary copy of the current conversation so we can delete any
Expand Down Expand Up @@ -3116,40 +3103,16 @@ def add_reply(self, reply: Union[DraftReply, Reply], sender: User, index: int) -
self.scroll.add_widget_to_conversation(index, conversation_item, Qt.AlignRight)
self.current_messages[reply.uuid] = conversation_item

def add_reply_from_reply_box(self, uuid: str, content: str) -> None:
"""
Add a reply from the reply box.
"""
if not self.controller.authenticated_user:
logger.error("User is no longer authenticated so cannot send reply.")
return

index = len(self.current_messages)
conversation_item = ReplyWidget(
self.controller,
uuid,
content,
"PENDING",
self.controller.reply_ready,
self.controller.reply_download_failed,
self.controller.reply_succeeded,
self.controller.reply_failed,
index,
self.scroll.widget().width(),
self.controller.authenticated_user,
True,
)
self.scroll.add_widget_to_conversation(index, conversation_item, Qt.AlignRight)
self.current_messages[uuid] = conversation_item

def on_reply_sent(self, source_uuid: str, reply_uuid: str, reply_text: str) -> None:
def on_reply_sent(self, source_uuid: str) -> None:
"""
Add the reply text sent from ReplyBoxWidget to the conversation.
"""
self.reply_flag = True
if source_uuid == self.source.uuid:
self.add_reply_from_reply_box(reply_uuid, reply_text)
self.update_deletion_markers()
try:
self.update_conversation(self.source.collection)
except sqlalchemy.exc.InvalidRequestError as e:
logger.debug(e)


class SourceConversationWrapper(QWidget):
Expand Down Expand Up @@ -3291,7 +3254,7 @@ class ReplyBoxWidget(QWidget):
A textbox where a journalist can enter a reply.
"""

reply_sent = pyqtSignal(str, str, str)
reply_sent = pyqtSignal(str)

def __init__(self, source: Source, controller: Controller) -> None:
super().__init__()
Expand Down Expand Up @@ -3388,7 +3351,7 @@ def send_reply(self) -> None:
self.text_edit.setText("")
reply_uuid = str(uuid4())
self.controller.send_reply(self.source.uuid, reply_uuid, reply_text)
self.reply_sent.emit(self.source.uuid, reply_uuid, reply_text)
self.reply_sent.emit(self.source.uuid)

@pyqtSlot(bool)
def _on_authentication_changed(self, authenticated: bool) -> None:
Expand Down
125 changes: 8 additions & 117 deletions tests/gui/test_widgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1940,41 +1940,6 @@ def test_SourceWidget_set_snippet(mocker, session_maker, session, homedir):
sw.set_snippet(source_uuid, "mock_file_uuid", "something new")


def test_SourceWidget_set_snippet_does_not_update_if_sync_is_stale(mocker):
"""
Ensure that the snippet does not get updated when an update is based on a sync that started
before a deletion operation succeeded.
"""
controller = mocker.MagicMock()
source = mocker.MagicMock()
source.journalist_designation = "Testy McTestface"
source.collection = [factory.Message(content="abc123")]
source.server_collection = source.collection
sw = SourceWidget(controller, source, mocker.MagicMock(), mocker.MagicMock())
sw.update()
assert sw.preview.text() == "abc123"

# Make the sync stale
sw.sync_started_timestamp = datetime.now()
sw.deletion_scheduled_timestamp = datetime.now()

# Set up source data so that it appears that the conversation was deleted
source.collection = []
source.interaction_count = 1
source.server_collection = source.collection

# Assert the preview snippet does not get updated because the sync is stale
sw.set_snippet(source.uuid, "mock_file_uuid")
assert sw.preview.text() == "abc123"

# The sync is no longer stale so the preview should show that the are no more source
# conversation items because they've been deleted
sw.deletion_scheduled_timestamp = datetime.now()
sw.sync_started_timestamp = datetime.now()
sw.set_snippet(source.uuid, "mock_file_uuid")
assert sw.preview.text() == "— All files and messages deleted for this source —"


def test_SourceWidget_update_truncate_latest_msg(mocker):
"""
If the latest message in the conversation is longer than 150 characters,
Expand Down Expand Up @@ -4325,22 +4290,6 @@ def test_ConversationView__on_conversation_deletion_successful_does_not_hide_dra
assert not cv.current_messages[draft_reply.uuid].isHidden()


def test_ConversationView_update_conversation_skips_if_sync_is_stale(mocker):
"""
If the sync started before the source was scheduled for deletion, do not update the conversation
"""
cv = ConversationView(factory.Source(), mocker.MagicMock())
cv.update_deletion_markers = mocker.MagicMock()
cv.sync_started_timestamp = datetime.now()
cv.deletion_scheduled_timestamp = datetime.now()
cv.update_conversation([])
cv.update_deletion_markers.assert_not_called()
# Also test that a new message will not get added if the sync is stale
cv.update_conversation([factory.Message()])
assert not cv.current_messages
cv.update_deletion_markers.assert_not_called()


def test_ConversationView_update_conversation_position_follow(mocker, homedir):
"""
Check the signal handler sets the correct value for the scrollbar to be
Expand Down Expand Up @@ -4448,12 +4397,12 @@ def test_ConversationView_on_reply_sent(mocker):
controller = mocker.MagicMock()
controller.authenticated_user = factory.User()
cv = ConversationView(source, controller)
cv.add_reply_from_reply_box = mocker.MagicMock()
cv.update_conversation = mocker.MagicMock()

assert cv.reply_flag is False
cv.on_reply_sent(source_uuid=source.uuid, reply_uuid="abc123", reply_text="test message")
cv.on_reply_sent(source_uuid=source.uuid)

cv.add_reply_from_reply_box.assert_called_with("abc123", "test message")
cv.update_conversation.assert_called_with(source.collection)
assert cv.reply_flag is True


Expand All @@ -4467,88 +4416,30 @@ def test_ConversationView_on_reply_sent_does_not_add_message_intended_for_differ
cv = ConversationView(source, controller)
cv.add_reply = mocker.MagicMock()

cv.on_reply_sent("different_source_id", "mock", "mock")
cv.on_reply_sent("different_source_id")

assert not cv.add_reply.called


def test_ConversationView_on_reply_sent_with_deleted_source(mocker):
"""
Check that replying to a deleted source causes an error and is logged.
Check that replying to a deleted source handles exception.
"""
source = factory.Source()
controller = mocker.MagicMock()
cv = ConversationView(source, controller)
cv.add_reply_from_reply_box = mocker.MagicMock()
cv.update_conversation = mocker.MagicMock()

def collection_error():
raise sqlalchemy.exc.InvalidRequestError()

debug_logger = mocker.patch("securedrop_client.gui.widgets.logger.debug")
mock_collection = mocker.patch(
"securedrop_client.db.Source.collection", new_callable=PropertyMock
)
ire = sqlalchemy.exc.InvalidRequestError()
mock_collection.side_effect = ire

cv.on_reply_sent(source.uuid, "mock", "mock")

debug_logger.assert_any_call(
f"Could not Update deletion markers in the ConversationView: {ire}"
)


def test_ConversationView_add_reply_from_reply_box(mocker, session, session_maker, homedir):
"""
Adding a reply from reply box results in a new ReplyWidget added to the layout.
"""
controller = logic.Controller("http://localhost", mocker.MagicMock(), session_maker, homedir)
controller.authenticated_user = factory.User()

source = factory.Source()
session.add(source)
session.commit()
cv = ConversationView(source, controller)
cv.add_reply_from_reply_box(uuid="abc123", content=">^..^<")

# Check that we added the correct widget to the layout
reply_widget = cv.scroll.conversation_layout.itemAt(0).widget()
assert isinstance(reply_widget, ReplyWidget)

assert reply_widget.uuid == "abc123"
assert reply_widget.message.text() == ">^..^<"
assert reply_widget.index == 0
assert reply_widget.status == "PENDING"
assert not reply_widget.failed_to_decrypt
assert reply_widget.controller == controller
assert reply_widget.sender_is_current_user
assert cv.current_messages["abc123"].sender == controller.authenticated_user
assert reply_widget.sender == controller.authenticated_user


def test_ConversationView_add_reply_from_reply_box_does_not_send_when_not_authenticated(mocker):
"""
Ensure we do not send a reply from reply box when the user is no longer authenticated.
"""
source = factory.Source()
reply_ready = mocker.MagicMock()
reply_download_failed = mocker.MagicMock()
reply_succeeded = mocker.MagicMock()
reply_failed = mocker.MagicMock()
controller = mocker.MagicMock(
reply_ready=reply_ready,
reply_download_failed=reply_download_failed,
reply_succeeded=reply_succeeded,
reply_failed=reply_failed,
)
controller.authenticated_user = None
cv = ConversationView(source, controller)
cv.scroll.conversation_layout = mocker.MagicMock()
reply_widget = mocker.patch("securedrop_client.gui.widgets.ReplyWidget")
cv.add_reply_from_reply_box(uuid="abc123", content="test message")

reply_widget.assert_not_called()
cv.scroll.conversation_layout.insertWidget.assert_not_called()
cv.on_reply_sent(source.uuid)


def test_ConversationView_add_reply(mocker, homedir, session, session_maker):
Expand Down Expand Up @@ -4825,7 +4716,7 @@ def test_ReplyBoxWidget_send_reply(mocker):

scw.reply_box.send_reply()

scw.reply_box.reply_sent.emit.assert_called_once_with("abc123", "456xyz", "Alles für Alle")
scw.reply_box.reply_sent.emit.assert_called_once_with("abc123")
scw.reply_box.text_edit.setText.assert_called_once_with("")
controller.send_reply.assert_called_once_with("abc123", "456xyz", "Alles für Alle")

Expand Down