-
Notifications
You must be signed in to change notification settings - Fork 42
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
Changes from all commits
196c969
c281524
9958697
1325068
935de8a
73e13b7
dc878cd
c3fb666
4880707
03c116b
95db9b4
44c4394
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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') |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
from securedrop_client.storage import update_draft_replies | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
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 | ||
|
@@ -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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
with an example I actually did call the |
||
collection.sort(key=lambda x: (x.file_counter, | ||
getattr(x, "timestamp", | ||
datetime.datetime(datetime.MINYEAR, 1, 1)))) | ||
return collection | ||
|
||
|
||
|
@@ -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""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I could imagine this is a place to store more detailed status codes about the failures in the future like |
||
PENDING = 'PENDING' | ||
FAILED = 'FAILED' | ||
|
||
|
||
class User(Base): | ||
|
||
__tablename__ = 'users' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so organized!