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

app: add pending reply status, persist replies in the database #578

Merged
merged 12 commits into from
Nov 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
67 changes: 67 additions & 0 deletions alembic/versions/86b01b6290da_add_reply_draft.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
"""add reply draft

Revision ID: 86b01b6290da
Revises: 36a79ffcfbfb
Create Date: 2019-10-17 09:45:07.643076

"""
from alembic import op
import sqlalchemy as sa


# revision identifiers, used by Alembic.
revision = '86b01b6290da'
down_revision = '36a79ffcfbfb'
branch_labels = None
depends_on = None


def upgrade():
op.create_table('replysendstatuses',
sa.Column('id', sa.Integer(), nullable=False),
sa.Column('name', sa.String(length=36), nullable=False),
sa.PrimaryKeyConstraint('id', name=op.f('pk_replysendstatuses')),
sa.UniqueConstraint('name', name=op.f('uq_replysendstatuses_name'))
)

# Set the initial in-progress send statuses: PENDING, FAILED
conn = op.get_bind()
conn.execute('''
INSERT INTO replysendstatuses
('name')
VALUES
('PENDING'),
('FAILED');
''')

op.create_table(
'draftreplies',
sa.Column('id', sa.Integer(), nullable=False),
sa.Column('uuid', sa.String(length=36), nullable=False),
sa.Column('timestamp', sa.DateTime(), nullable=False),
sa.Column('source_id', sa.Integer(), nullable=False),
sa.Column('journalist_id', sa.Integer(), nullable=True),
sa.Column('file_counter', sa.Integer(), nullable=False),
sa.Column('content', sa.Text(), nullable=True),
sa.Column('send_status_id', sa.Integer(), nullable=True),
sa.PrimaryKeyConstraint('id', name=op.f('pk_draftreplies')),
sa.UniqueConstraint('uuid', name=op.f('uq_draftreplies_uuid')),
sa.ForeignKeyConstraint(
['source_id'],
['sources.id'],
name=op.f('fk_draftreplies_source_id_sources')),
sa.ForeignKeyConstraint(
['journalist_id'],
['users.id'],
name=op.f('fk_draftreplies_journalist_id_users')),
sa.ForeignKeyConstraint(
['send_status_id'],
['replysendstatuses.id'],
op.f('fk_draftreplies_send_status_id_replysendstatuses'),
)
)


def downgrade():
op.drop_table('draftreplies')
op.drop_table('replysendstatuses')
13 changes: 12 additions & 1 deletion create_dev_data.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
#!/usr/bin/env python3
import json
import os
from sqlalchemy.orm.exc import NoResultFound
import sys

from securedrop_client.config import Config
from securedrop_client.db import Base, make_session_maker
from securedrop_client.db import Base, make_session_maker, ReplySendStatus, ReplySendStatusCodes

sdc_home = sys.argv[1]
session = make_session_maker(sdc_home)()
Expand All @@ -13,3 +15,12 @@
f.write(json.dumps({
'journalist_key_fingerprint': '65A1B5FF195B56353CC63DFFCC40EF1228271441',
}))

for reply_send_status in ReplySendStatusCodes:
try:
reply_status = session.query(ReplySendStatus).filter_by(
name=reply_send_status.value).one()
except NoResultFound:
reply_status = ReplySendStatus(reply_send_status.value)
session.add(reply_status)
session.commit()
48 changes: 43 additions & 5 deletions securedrop_client/api_jobs/uploads.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

from securedrop_client.api_jobs.base import ApiJob
from securedrop_client.crypto import GpgHelper
from securedrop_client.db import Reply, Source
from securedrop_client.db import DraftReply, Reply, ReplySendStatus, ReplySendStatusCodes, Source
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so organized!

from securedrop_client.storage import update_draft_replies

logger = logging.getLogger(__name__)

Expand All @@ -28,28 +29,65 @@ def call_api(self, api_client: API, session: Session) -> str:
we can return the reply uuid.
'''
try:
encrypted_reply = self.gpg.encrypt_to_source(self.source_uuid, self.message)
sdk_reply = self._make_call(encrypted_reply, api_client)
draft_reply_db_object = session.query(DraftReply).filter_by(uuid=self.reply_uuid).one()
source = session.query(Source).filter_by(uuid=self.source_uuid).one()
session.commit()

encrypted_reply = self.gpg.encrypt_to_source(self.source_uuid, self.message)
interaction_count = source.interaction_count + 1
filename = '{}-{}-reply.gpg'.format(interaction_count,
source.journalist_designation)
reply_db_object = Reply(
uuid=self.reply_uuid,
source_id=source.id,
filename=filename,
journalist_id=api_client.token_journalist_uuid,
filename=sdk_reply.filename,
content=self.message,
is_downloaded=True,
is_decrypted=True
is_decrypted=True,
sssoleileraaa marked this conversation as resolved.
Show resolved Hide resolved
)
sdk_reply = self._make_call(encrypted_reply, api_client)

# Update filename and file_counter in case they changed on the server
new_file_counter = int(sdk_reply.filename.split('-')[0])
reply_db_object.file_counter = new_file_counter
reply_db_object.filename = sdk_reply.filename

draft_file_counter = draft_reply_db_object.file_counter
draft_timestamp = draft_reply_db_object.timestamp

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()

return reply_db_object.uuid
except RequestTimeoutError as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is out of the scope of this PR but shouldn't we also be catching AuthError and ApiInaccessibleError and raising a custom exception to include reply_uuid and message like we do SendReplyJobTimeoutError?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:o yes... yes we should

message = "Failed to send reply for source {id} due to Exception: {error}".format(
id=self.source_uuid, error=e)

# Update draft reply send status to FAILED
reply_status = session.query(ReplySendStatus).filter_by(
name=ReplySendStatusCodes.FAILED.value).one()
draft_reply_db_object.send_status_id = reply_status.id
session.add(draft_reply_db_object)
session.commit()

raise SendReplyJobTimeoutError(message, self.reply_uuid)
except Exception as e:
message = "Failed to send reply for source {id} due to Exception: {error}".format(
id=self.source_uuid, error=e)

# Update draft reply send status to FAILED
reply_status = session.query(ReplySendStatus).filter_by(
name=ReplySendStatusCodes.FAILED.value).one()
draft_reply_db_object.send_status_id = reply_status.id
session.add(draft_reply_db_object)
session.commit()

raise SendReplyJobError(message, self.reply_uuid)

def _make_call(self, encrypted_reply: str, api_client: API) -> sdclientapi.Reply:
Expand Down
63 changes: 62 additions & 1 deletion securedrop_client/db.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import datetime
from enum import Enum
import os

from typing import Any, List, Union # noqa: F401
Expand Down Expand Up @@ -54,7 +56,11 @@ def collection(self) -> List:
collection.extend(self.messages)
collection.extend(self.files)
collection.extend(self.replies)
collection.sort(key=lambda x: x.file_counter)
collection.extend(self.draftreplies)
# Sort first by the file_counter, then by timestamp (used only for draft replies).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's a more high-level visible place we should mention how we use timestamps for saved drafts. I can't think of one other than in a docstring for source or in our client architecture doc. I was just hoping to find more information about why we use the timestamp somewhere. I did find your PR comment:

this is the local timestamp that the reply was sent and this will be used for ordering local replies when there are multiple attempted replies in between conversation items from the server.

So we could use a local_file_counter instead of timestamp right? I don't feel strongly about this but maybe it makes it clearer that we don't actually care about time, we just care about order in which a reply was drafted so that we can display the drafts in the correct order in the client?

Or perhaps we'll want to show the timestamp next to the draft to help the journalist remember when they drafted it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm yeah good point - how about I add a description of the ordering situation here to the wiki architecture page?

i.e. saying something like

"draft replies store:

  • a file_counter which points to the file_counter of the previously sent item. this enables us to interleave the drafts with the items from the source conversation fetched from the server, which do not have timestamps associated with them.
  • a timestamp which contains the timestamp the draft reply was saved locally: this is used to order drafts in the case where there are multiple drafts sent after a given reply (i.e. when file_counter is the same for multiple drafts)"

with an example

I actually did call the DraftReply.file_counter field local_file_counter field (😇) but then renamed it back to file_counter to simplify the source.collection.sort key. You're right that we could ditch timestamp and have two fields file_counter and local_file_counter. imho I figure is slightly more useful to have the actual timestamp locally for if we ever do want to expose the draft timestamp to users (I could imagine that being useful).

collection.sort(key=lambda x: (x.file_counter,
getattr(x, "timestamp",
datetime.datetime(datetime.MINYEAR, 1, 1))))
return collection


Expand Down Expand Up @@ -214,6 +220,61 @@ def __repr__(self) -> str:
return '<Reply {}>'.format(self.filename)


class DraftReply(Base):

__tablename__ = 'draftreplies'

id = Column(Integer, primary_key=True)
uuid = Column(String(36), unique=True, nullable=False)
timestamp = Column(DateTime, nullable=False)
source_id = Column(Integer, ForeignKey('sources.id'), nullable=False)
source = relationship("Source",
backref=backref("draftreplies", order_by=id,
cascade="delete"))
journalist_id = Column(Integer, ForeignKey('users.id'))
journalist = relationship(
"User", backref=backref('draftreplies', order_by=id))

# Tracks where in this conversation the draft reply was sent.
# This points to the file_counter of the previous conversation item.
file_counter = Column(Integer, nullable=False)
content = Column(Text)

# This tracks the sending status of the reply.
send_status_id = Column(
Integer,
ForeignKey('replysendstatuses.id')
)
send_status = relationship("ReplySendStatus")

def __init__(self, **kwargs: Any) -> None:
super().__init__(**kwargs)

def __repr__(self) -> str:
return '<DraftReply {}>'.format(self.uuid)


class ReplySendStatus(Base):

__tablename__ = 'replysendstatuses'

id = Column(Integer, primary_key=True)
name = Column(String(36), unique=True, nullable=False)

def __init__(self, name: str) -> None:
super().__init__()
self.name = name

def __repr__(self) -> str:
return '<Reply status {}>'.format(self.name)


class ReplySendStatusCodes(Enum):
"""In progress (sending) replies can currently have the following statuses"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a SAVED status in the future 🔮

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha yeah so at some point in this PR before I created the DraftReply object this status object was stored on the Reply and had three codes: FAILED, SUCCESSFUL, PENDING. but I removed SUCCESSFUL when I created the DraftReply as only drafts can be FAILED or PENDING, all replies are by construction SUCCESSFUL

I could imagine this is a place to store more detailed status codes about the failures in the future like ENCRYPTION_FAILED_NO_SOURCE_KEY, SEND_FAILED_SOURCE_DELETED. We could also imagine some more granular pending statuses like PENDING_ENCRYPTION_IN_PROGRESS, PENDING_AWAITING_SERVER_RESPONSE.

PENDING = 'PENDING'
FAILED = 'FAILED'


class User(Base):

__tablename__ = 'users'
Expand Down
Loading