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

Feature/support multiple internal users #140

Merged
merged 11 commits into from
Feb 19, 2018
Merged

Feature/support multiple internal users #140

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
019f5cf
initial commit - wip - for visibility by other devs. tests not passing
Feb 6, 2018
9561043
more wip to share with other devs
Feb 7, 2018
584d528
wip
Feb 7, 2018
2c74355
existing tests passing with support for internal user searches not us…
Feb 8, 2018
9ed911a
Removed unused versioning code
Feb 8, 2018
9bb9a02
removed unused versioning code
Feb 8, 2018
f11c500
Merge with Master
Feb 8, 2018
64a34b2
fix merge conflicts
Feb 8, 2018
c53446f
fixing inadvertent change from 'database' to 'repository' in strings
Feb 8, 2018
f08afb4
changes for pr comments
Feb 13, 2018
5f8f449
Merge branch 'master' into feature/support-multiple-internal-users
Feb 15, 2018
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
2 changes: 1 addition & 1 deletion .pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ max-nested-blocks=5
[FORMAT]

# Maximum number of characters on a single line.
max-line-length=150
max-line-length=160

# Regexp for a line that is allowed to be longer than the limit.
ignore-long-lines=^\s*(# )?<?https?://\S+>?$
Expand Down
6 changes: 3 additions & 3 deletions run_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
os.environ['APP_SETTINGS'] = 'TestConfig'

from behave import __main__ as behave_executable
behave = behave_executable.main()
behave_errors = behave_executable.main()

test_dirs = os.listdir('./tests')
suites_list = []
Expand All @@ -18,6 +18,6 @@
suite = loader.discover(test_path)
suites_list.append(suite)
result = unittest.TextTestRunner(verbosity=2).run(suite)
i = len(result.failures) + len(result.errors)
if i != 0 or behave == 1:
if result.failures or result.errors or behave_errors:
sys.exit(1)

48 changes: 42 additions & 6 deletions secure_message/api_mocks/internal_user_service_mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,50 @@

class InternalUserServiceMock:
def __init__(self):
self.internal_user_dict = {"Someuuid": {"id": "Someuuid",
"firstName": "fred",
"lastName": "flinstone",
"emailAddress": "mock@email.com"}}
self.internal_user_dict = {
"Someuuid": {
"id": "Someuuid",
"firstName": "fred",
"lastName": "flinstone",
"emailAddress": "mock@email.com"
},
"01b51fcc-ed43-4cdb-ad1c-450f9986859b": {
"id": "01b51fcc-ed43-4cdb-ad1c-450f9986859b",
"firstName": "fred",
"lastName": "flinstone",
"emailAddress": "mock@email.com"
},
"f62dfda8-73b0-4e0e-97cf-1b06327a6712": {
"id": "01b51fcc-ed43-4cdb-ad1c-450f9986859b",
"firstName": "fred",
"lastName": "flinstone",
"emailAddress": "mock@email.com"
},
"Tej": {
"id": "Tej",
"firstName": "Tejas",
"lastName": "patel",
"emailAddress": "mock@email.com"
},
"BRES": {
"id": "TRES",
"firstName": "BRES",
"lastName": "",
"emailAddress": "mock@email.com"
},
"SomeStringUntilWeGetIds": {
"id": "SomeStringUntilWeGetIds",
"firstName": "MadeUpUser",
"lastName": "",
"emailAddress": "mock@email.com"
}
}

def get_user_details(self, uuid):
"""gets the user details from the internal user service"""
logger.debug("getting mock user details for uaa")
if uuid:
return self.internal_user_dict[uuid], 200
return "error retrieving details"
found = self.internal_user_dict.get(uuid)
if found:
return found, 200
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have done like :
if uuid in elf.internal_user_dict
return true, 200

return False, 400

just for consistency in the return type. and log with error/warning user not found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try and use EAFP instead of LBYL? https://blogs.msdn.microsoft.com/pythonengineering/2016/06/29/idiomatic-python-eafp-versus-lbyl/

try:
    return self.internal_user_dict[uuid], 200
except KeyError:
    logger.exception(f'No internal user with uuid {uuid}'
    return "error retrieving details", 404

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, don't inspect the uuid arg. If it's None then we'll still catch the exception, and if we set None as a key in a dictionary we have bigger problems.

Copy link
Author

Choose a reason for hiding this comment

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

Normally I would support EAFP instead of LBYL . However in this case the existing case and party mocks are written so as to mimic the http response which would not raise an exception to the code but return an error . In this specific of the internal_user_service_mock it is something of a guess to assume it will be a http response . If it isnt and we do get exceptions I will change the mock implementation to suit. The uuid in elf.internal_user_dict needs to look in the values()

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion doesn't change the return values, only the way in which the decision for what values to return is made

Copy link
Author

Choose a reason for hiding this comment

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

ok , misunderstood , changed

return "error retrieving details", 404
24 changes: 2 additions & 22 deletions secure_message/application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,9 @@

from secure_message.exception.exceptions import MissingEnvironmentVariable
from secure_message.repository import database
from secure_message.resources.health import Health, DatabaseHealth, HealthDetails
from secure_message.resources.info import Info
from secure_message.resources.labels import Labels
from secure_message.resources.messages import MessageList, MessageSend, MessageById, MessageModifyById
from secure_message.authentication.authenticator import authenticate
from secure_message.resources.drafts import DraftSave, DraftById, DraftModifyById, DraftList
from secure_message.resources.threads import ThreadById, ThreadList
from secure_message.logger_config import logger_initial_config

from secure_message.v1.application import set_v1_resources

logger_initial_config(service_name='ras-secure-message')
logger = wrap_logger(logging.getLogger(__name__))
Expand Down Expand Up @@ -48,21 +42,7 @@ def create_app(config=None):
database.db.create_all()
database.db.session.commit() # NOQA pylint:disable=no-member

api.add_resource(Health, '/health')
api.add_resource(DatabaseHealth, '/health/db')
api.add_resource(HealthDetails, '/health/details')
api.add_resource(Info, '/info')
api.add_resource(MessageList, '/messages')
api.add_resource(MessageSend, '/message/send')
api.add_resource(MessageById, '/message/<message_id>')
api.add_resource(MessageModifyById, '/message/<message_id>/modify')
api.add_resource(DraftSave, '/draft/save')
api.add_resource(DraftModifyById, '/draft/<draft_id>/modify')
api.add_resource(DraftById, '/draft/<draft_id>')
api.add_resource(ThreadById, '/thread/<thread_id>')
api.add_resource(DraftList, '/drafts')
api.add_resource(ThreadList, '/threads')
api.add_resource(Labels, '/labels')
set_v1_resources(api)

@app.before_request
def before_request(): # NOQA pylint:disable=unused-variable
Expand Down
12 changes: 3 additions & 9 deletions secure_message/repository/database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,7 @@ def serialize(self, user, body_summary=False):
'_links': '',
'labels': []}

if user.is_internal:
actor = constants.BRES_USER
else:
actor = user.user_uuid

self._populate_to_and_from(actor, message)
self._populate_to_and_from(user, message)

self._populate_events(message)

Expand All @@ -100,11 +95,10 @@ def _populate_events(self, message):
elif row.event == EventsApi.READ.value:
message['read_date'] = str(row.date_time)

def _populate_to_and_from(self, actor, message):
def _populate_to_and_from(self, user, message):
for row in self.statuses:
if row.actor == actor:
if row.actor == user.user_uuid or (user.is_internal and (row.actor == constants.NON_SPECIFIC_INTERNAL_USER or row.actor == constants.BRES_USER)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this here for the purposes of passing tests?

Copy link
Author

Choose a reason for hiding this comment

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

No , its needed so that the status's are retrieved correctly in the case that a respondent sends a message to a group . If the user is internal we want to see the status's even if the message was sent to a group .

message['labels'].append(row.label)

if row.label == Labels.INBOX.value:
message['msg_to'].append(row.actor)
elif row.label == Labels.SENT.value:
Expand Down
4 changes: 3 additions & 1 deletion secure_message/repository/modifier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,18 @@ def del_draft(draft_id):
del_draft_status = "DELETE FROM securemessage.status WHERE msg_id='{0}' AND label='{1}'".format(draft_id, Labels.DRAFT.value)
del_draft_event = "DELETE FROM securemessage.events WHERE msg_id='{0}'".format(draft_id)
del_draft_inbox_status = "DELETE FROM securemessage.status WHERE msg_id='{0}' AND label='{1}'".format(draft_id, Labels.DRAFT_INBOX.value)
del_actors = "DELETE FROM securemessage.actors where msg_id='{0}'".format(draft_id)
del_draft_msg = "DELETE FROM securemessage.secure_message WHERE msg_id='{0}'".format(draft_id)

try:
db.get_engine(app=db.get_app()).execute(del_draft_status)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this where we should be using db.session?

Copy link
Author

Choose a reason for hiding this comment

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

Possibly , I must admit I followed what was there. Not sure of the implications of changing to that . The draft is normally deleted in the context of sending the same message as a draft .Other functions in the same class do use the session so Its possible that the two might interfere with each other , but unless I did an investigation I could not be sure .

Copy link
Contributor

@JamesGardiner JamesGardiner Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Session should be used. It encapsulates db.get_engine. See http://flask-sqlalchemy.pocoo.org/2.3/api/#sessions

Copy link
Author

Choose a reason for hiding this comment

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

Added as a tech debt issue

db.get_engine(app=db.get_app()).execute(del_draft_inbox_status)
db.get_engine(app=db.get_app()).execute(del_draft_event)
db.get_engine(app=db.get_app()).execute(del_actors)
Copy link
Contributor

Choose a reason for hiding this comment

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

could we just call get_engine once?

Copy link
Author

Choose a reason for hiding this comment

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

I must admit I am not sure , I followed on from what was there before . I can look at this

Copy link
Contributor

Choose a reason for hiding this comment

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

Think James comment above supersedes this now

Copy link
Author

Choose a reason for hiding this comment

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

tech debt ticket added

db.get_engine(app=db.get_app()).execute(del_draft_msg)

except Exception 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.

Probably worth changing this to logger.exception while we are at it.

Copy link
Author

Choose a reason for hiding this comment

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

done

logger.error('Error deleting draft from database', msg_id=draft_id, error=e)
logger.exception('Error deleting draft from database', msg_id=draft_id, error=e)
Copy link
Contributor

Choose a reason for hiding this comment

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

logger.exception looks up the stack trace to find the exception so error=e is not needed

Copy link
Author

Choose a reason for hiding this comment

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

done

raise InternalServerError(description="Error deleting draft from database")

@staticmethod
Expand Down
123 changes: 97 additions & 26 deletions secure_message/repository/retriever.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,22 @@

class Retriever:
"""Created when retrieving messages"""

@staticmethod
def retrieve_message_list(page, limit, user, ru_id=None, survey=None, cc=None, ce=None, label=None, descend=True):
"""returns list of messages from db"""
conditions = []
status_conditions = []

if user.is_respondent:
status_conditions.append(Status.actor == str(user.user_uuid))
else:
status_conditions.append(Status.actor == constants.BRES_USER)
return Retriever._retrieve_message_list_respondent(page, limit, user=user, ru_id=ru_id, survey=survey,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the python way to do things, but when there ar eso many parameters is not better to encapsulate them all in a Object ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be made cleaner by refactoring the is not None below to just do dict key access on kwargs passed here. If a KeyError is raised, you know the key isn't passed and therefore can do the else condition from below.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with possibly moving the parameters to kwargs . However this how the existing code handles it. So whilst I agree its best to follow the boy scout principle , in this case I will raise a ticket to loop back depending on time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the named tuple created in the calling function instead? It seems to create the Named Tuple when the request arrives but then unpack it to seperate variables in order to call these function when actually everything you need (apart from the user) is in the tuple

Copy link
Author

Choose a reason for hiding this comment

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

I am putting this on a tech debt ticket that I plan on coming back to asap

cc=cc, ce=ce, label=label, descend=descend)
return Retriever._retrieve_message_list_internal(page, limit, ru_id=ru_id, survey=survey,
cc=cc, ce=ce, label=label, descend=descend)

@staticmethod
def _retrieve_message_list_respondent(page, limit, user, ru_id, survey, cc, ce, label, descend):
"""returns list of messages from db"""
conditions = []
status_conditions = [Status.actor == str(user.user_uuid)]

if label is not None:
status_conditions.append(Status.label == str(label))
Expand All @@ -46,38 +52,95 @@ def retrieve_message_list(page, limit, user, ru_id=None, survey=None, cc=None, c

try:
t = db.session.query(SecureMessage.msg_id, func.max(Events.date_time) # pylint:disable=no-member
.label('max_date'))\
.label('max_date')) \
.join(Events).join(Status).outerjoin(Actors) \
.filter(and_(*conditions)) \
.filter(and_(*status_conditions)) \
.filter(or_(Events.event == EventsApi.SENT.value, Events.event == EventsApi.DRAFT_SAVED.value)) \
.group_by(SecureMessage.msg_id).subquery('t')

if descend:
Copy link
Contributor

@JamesGardiner JamesGardiner Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If you follow my approach above set this to be a default kwarg.

Copy link
Author

Choose a reason for hiding this comment

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

I have raised a ticket to look at cleaning this up as mentioned above . I agree it can be improved

result = SecureMessage.query\
.filter(SecureMessage.msg_id == t.c.msg_id) \
.order_by(t.c.max_date.desc()).paginate(page, limit, False)

order = t.c.max_date.desc()
else:
result = SecureMessage.query \
.filter(SecureMessage.msg_id == t.c.msg_id) \
.order_by(t.c.max_date.asc()).paginate(page, limit, False)
order = t.c.max_date.asc()

result = SecureMessage.query \
.filter(SecureMessage.msg_id == t.c.msg_id) \
.order_by(order).paginate(page, limit, False)

except Exception as e:
logger.error('Error retrieving messages from database', error=e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a logger.exception?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch , I copied existing code , changing

raise InternalServerError(description="Error retrieving messages from database")

return True, result

@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really confused to why these methods return True, result?

The calling function does an if on the status which is always true?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch . Me too , changed.

Copy link
Author

Choose a reason for hiding this comment

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

Actually tried to change as I agree it looks wrong . Get lots of failing tests . Raised a tech debt ticket . Plan on looping back to this

def _retrieve_message_list_internal(page, limit, ru_id, survey, cc, ce, label, descend):
"""returns list of messages from db"""
conditions = []
status_reject_conditions = []
valid_statuses = []
actor_conditions = []

if label is not None:
valid_statuses.append(label)
if label in [Labels.INBOX.value, Labels.ARCHIVE.value, Labels.UNREAD.value]:
actor_conditions.append(Actors.sent_from_internal == False) # NOQA pylint:disable=singleton-comparison
Copy link
Contributor

Choose a reason for hiding this comment

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

This to me sums up why the DB structure isn't correct because it shouldn't be this complicated to determine if the internal user has sent or received the message

Copy link
Author

Choose a reason for hiding this comment

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

Thats not what its doing . Its adding predicates specifically only when someone has asked for only messages with specific labels to be returned . The simple sent_from_internal flag tells us if it was sent by an internal person

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems I misunderstood it then

if label in [Labels.DRAFT.value, Labels.SENT.value]:
actor_conditions.append(Actors.sent_from_internal == True) # NOQA pylint:disable=singleton-comparison
else:
status_reject_conditions.append(Labels.DRAFT_INBOX.value)
valid_statuses = [Labels.INBOX.value, Labels.DRAFT.value]
actor_conditions.append(True)

if ru_id is not None:
conditions.append(SecureMessage.ru_id == str(ru_id))

if survey is not None:
conditions.append(SecureMessage.survey == str(survey))

if cc is not None:
conditions.append(SecureMessage.collection_case == str(cc))

if ce is not None:
conditions.append(SecureMessage.collection_exercise == str(ce))

try:
t = db.session.query(SecureMessage.msg_id, func.max(Events.date_time) # pylint:disable=no-member ~ below used to obtain not in
.label('max_date')) \
.join(Events).join(Status).outerjoin(Actors) \
.filter(and_(*conditions)) \
.filter(or_(*actor_conditions)) \
.filter(~Status.label.in_(status_reject_conditions)) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth a comment here that SQLAlchemy overrides the unary bitwise operator

Copy link
Author

Choose a reason for hiding this comment

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

Done

.filter(Status.label.in_(valid_statuses)) \
.filter(or_(Events.event == EventsApi.SENT.value, Events.event == EventsApi.DRAFT_SAVED.value)) \
.group_by(SecureMessage.msg_id).subquery('t')

if descend:
order = t.c.max_date.desc()
else:
order = t.c.max_date.asc()

result = SecureMessage.query \
.filter(SecureMessage.msg_id == t.c.msg_id) \
.order_by(order).paginate(page, limit, False)

except Exception as e:
logger.exception('Error retrieving messages from database', error=e)
raise InternalServerError(description="Error retrieving messages from database")

return True, result

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really confused to why these methods return True, result?

The calling functions do an if on the status but its always true so appears to be pointless, can we just remove it and return the result?

Copy link
Author

Choose a reason for hiding this comment

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

As above , done

@staticmethod
def unread_message_count(user):
"""Count users unread messages"""
status_conditions = []
status_conditions.append(Status.actor == str(user.user_uuid))
try:
result = SecureMessage.query.join(Status).filter(and_(*status_conditions)).filter(Status.label == 'UNREAD').count()
result = SecureMessage.query.join(Status).filter(and_(*status_conditions)).filter(
Status.label == 'UNREAD').count()
except Exception as e:
logger.error('Error retrieving count of unread messages from database', error=e)
logger.exception('Error retrieving count of unread messages from database', error=e)
Copy link
Contributor

Choose a reason for hiding this comment

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

as above no need to log error=e

Copy link
Author

Choose a reason for hiding this comment

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

done

raise InternalServerError(description="Error retrieving count of unread messages from database")
return result

Expand All @@ -86,33 +149,37 @@ def retrieve_thread_list(page, limit, user):
"""returns list of threads from db"""
status_conditions = []
conditions = []
actor_conditions = []

if user.is_respondent:
status_conditions.append(Status.actor == str(user.user_uuid))
actor_conditions.append(Status.actor == str(user.user_uuid))
else:
status_conditions.append(Status.actor == constants.BRES_USER)
actor_conditions.append(Status.actor == str(user.user_uuid))
actor_conditions.append(Status.actor == constants.BRES_USER)
actor_conditions.append(Status.actor == constants.NON_SPECIFIC_INTERNAL_USER)

status_conditions.append(Status.label != Labels.DRAFT_INBOX.value)

try:
t = db.session.query(SecureMessage.thread_id, func.max(Events.date_time) # pylint:disable=no-member
.label('max_date')) \
t = db.session.query(SecureMessage.thread_id, func.max(Events.id) # pylint:disable=no-member
.label('max_id')) \
.join(Events).join(Status) \
.filter(and_(*status_conditions)) \
.filter(or_(*actor_conditions)) \
.filter(or_(Events.event == EventsApi.SENT.value, Events.event == EventsApi.DRAFT_SAVED.value)) \
.group_by(SecureMessage.thread_id).subquery('t')

conditions.append(SecureMessage.thread_id == t.c.thread_id)
conditions.append(Events.date_time == t.c.max_date)
conditions.append(Events.id == t.c.max_id)

result = SecureMessage.query.join(Events).join(Status) \
.filter(or_(Events.event == EventsApi.SENT.value, Events.event == EventsApi.DRAFT_SAVED.value)) \
.filter(and_(*conditions)) \
.filter(and_(*status_conditions)) \
.order_by(t.c.max_date.desc()).paginate(page, limit, False)
.order_by(t.c.max_id.desc()).paginate(page, limit, False)

except Exception as e:
logger.error('Error retrieving messages from database', error=e)
logger.exception('Error retrieving messages from database', error=e)
raise InternalServerError(description="Error retrieving messages from database")

return True, result
Expand All @@ -137,11 +204,14 @@ def retrieve_message(message_id, user):
def retrieve_thread(thread_id, user, page, limit):
"""returns paginated list of messages for thread id"""
status_conditions = []
actor_conditions = []

if user.is_respondent:
status_conditions.append(Status.actor == str(user.user_uuid))
actor_conditions.append(Status.actor == str(user.user_uuid))
else:
status_conditions.append(Status.actor == constants.BRES_USER)
actor_conditions.append(Status.actor == str(user.user_uuid))
actor_conditions.append(Status.actor == constants.BRES_USER)
actor_conditions.append(Status.actor == constants.NON_SPECIFIC_INTERNAL_USER)

status_conditions.append(Status.label != Labels.DRAFT_INBOX.value)

Expand All @@ -150,6 +220,7 @@ def retrieve_thread(thread_id, user, page, limit):
result = SecureMessage.query.join(Events).join(Status) \
.filter(SecureMessage.thread_id == thread_id) \
.filter(and_(*status_conditions)) \
.filter(or_(*actor_conditions)) \
.filter(or_(Events.event == EventsApi.SENT.value, Events.event == EventsApi.DRAFT_SAVED.value)) \
.order_by(Events.date_time.desc()).paginate(page, limit, False)

Expand All @@ -168,7 +239,7 @@ def retrieve_draft(message_id, user):
"""returns single draft from db"""

try:
result = SecureMessage.query.filter(SecureMessage.msg_id == message_id)\
result = SecureMessage.query.filter(SecureMessage.msg_id == message_id) \
.filter(SecureMessage.statuses.any(Status.label == Labels.DRAFT.value)).first()
if result is None:
logger.error('Draft does not exist', message_id=message_id)
Expand Down Expand Up @@ -203,7 +274,7 @@ def check_db_connection():
def check_msg_id_is_a_draft(draft_id, user):
"""Check msg_id is that of a valid draft and return true/false if no ID is present"""
try:
result = SecureMessage.query.filter(SecureMessage.msg_id == draft_id)\
result = SecureMessage.query.filter(SecureMessage.msg_id == draft_id) \
.filter(SecureMessage.statuses.any(Status.label == Labels.DRAFT.value)).first()
except Exception as e:
logger.error('Error retrieving message from database', error=e)
Expand Down
Loading