Feature/support multiple internal users #140

AndrewTorrance · 2018-02-08T15:29:33Z

What is the context of this PR?
This supports the changing of the database such that the existing tests all pass . It is a preparatory step prior to supporting multiple internal users. The reason for the PR is that we need a breaking change in order to support multiple users and hence we need to version the api .

Added Actors column which supports identifying if an actor is internal or external. Added support for a message to a group by using the keyword 'GROUP' in the to field. ( could not use empty as that would fail validation in the case of a user not setting an actor)

…ing user id . I.e a prerequisite to having multiple internal users

atrozzo · 2018-02-12T14:48:19Z

secure_message/api_mocks/internal_user_service_mock.py

-        return "error retrieving details"
+            found = self.internal_user_dict.get(uuid)
+            if found:
+                return found, 200


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.

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

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.

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

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

ok , misunderstood , changed

atrozzo · 2018-02-12T14:53:17Z

secure_message/repository/retriever.py

-            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,


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 ?

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.

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.

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

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

rhiGriff · 2018-02-12T15:02:36Z

secure_message/repository/retriever.py

+                    .order_by(t.c.max_date.asc()).paginate(page, limit, False)
+
+        except Exception as e:
+            logger.error('Error retrieving messages from database', error=e)


Shouldn't this be a logger.exception?

Good catch , I copied existing code , changing

JamesGardiner

Only able to get to _retrieve_message_list_internal so far. Will finish tomorrow

JamesGardiner · 2018-02-12T16:18:17Z

secure_message/api_mocks/internal_user_service_mock.py

-        return "error retrieving details"
+            found = self.internal_user_dict.get(uuid)
+            if found:
+                return found, 200


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

JamesGardiner · 2018-02-12T16:20:07Z

secure_message/api_mocks/internal_user_service_mock.py

-        return "error retrieving details"
+            found = self.internal_user_dict.get(uuid)
+            if found:
+                return found, 200


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.

JamesGardiner · 2018-02-12T16:21:11Z

secure_message/repository/database.py

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


Is this here for the purposes of passing tests?

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 .

JamesGardiner · 2018-02-12T16:23:32Z

secure_message/repository/modifier.py

@@ -91,12 +91,14 @@ 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)


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

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 .

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

Added as a tech debt issue

JamesGardiner · 2018-02-12T16:26:49Z

secure_message/repository/modifier.py

        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)
            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)
            db.get_engine(app=db.get_app()).execute(del_draft_msg)

        except Exception as e:


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

JamesGardiner · 2018-02-12T16:35:44Z

secure_message/repository/retriever.py

-            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,


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.

JamesGardiner · 2018-02-12T16:36:55Z

secure_message/repository/retriever.py

@@ -46,15 +52,73 @@ 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:


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

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

JamesGardiner · 2018-02-12T16:42:58Z

secure_message/repository/retriever.py

+                .join(Events).join(Status).outerjoin(Actors) \
+                .filter(and_(*conditions)) \
+                .filter(or_(*actor_conditions)) \
+                .filter(~Status.label.in_(status_reject_conditions)) \


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

JamesGardiner · 2018-02-12T16:46:47Z

secure_message/repository/retriever.py

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


Sqlalchemy allows query building, so the duplicated code here can by completely removed apart from the .asc or .desc.

Or just do

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

Good catch , done

warrenbailey · 2018-02-12T16:49:57Z

secure_message/validation/user.py

-    def is_valid_user(uuid):
+    def is_valid_internal_user(uuid):
+        _, status_code = internal_user_service.get_user_details(uuid)
+        return True if status_code == 200 else False


can be simplified to

return status_code == 200

doh , thanks , done

warrenbailey · 2018-02-12T16:53:14Z

secure_message/repository/modifier.py

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


could we just call get_engine once?

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

Think James comment above supersedes this now

tech debt ticket added

warrenbailey · 2018-02-12T16:54:59Z

secure_message/validation/user.py

        _, status_code = party.get_user_details(uuid)
-        return status_code == 200
+        return True if status_code == 200 else False


warrenbailey · 2018-02-13T14:48:14Z

secure_message/repository/modifier.py

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


Think James comment above supersedes this now

warrenbailey · 2018-02-13T14:48:55Z

secure_message/repository/modifier.py

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

        except Exception as e:
-            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)


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

Happy with the changes to my comments

JamesGardiner · 2018-02-13T16:04:53Z

secure_message/v1/application.py

+
+
+def set_v1_resources(api):
+    api.add_resource(Health, '/health')


Are these going to be updated following the tech session review yesterday?

These are the v1 resources aren't they?

If you mean the health endpoint , I am not aware of any request to change that

warrenbailey · 2018-02-14T08:12:55Z

secure_message/repository/retriever.py

-            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,


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

warrenbailey · 2018-02-14T08:14:08Z

secure_message/repository/retriever.py


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

        return True, result

+    @staticmethod


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

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

Good catch . Me too , changed.

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

warrenbailey · 2018-02-14T08:14:37Z

secure_message/repository/retriever.py

+            raise InternalServerError(description="Error retrieving messages from database")
+
+        return True, result
+


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?

As above , done

warrenbailey · 2018-02-14T08:15:24Z

secure_message/repository/retriever.py

        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)


as above no need to log error=e

warrenbailey · 2018-02-14T08:16:54Z

secure_message/repository/retriever.py

+        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


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

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

Seems I misunderstood it then

warrenbailey · 2018-02-14T08:19:46Z

secure_message/v1/application.py

+
+
+def set_v1_resources(api):
+    api.add_resource(Health, '/health')


These are the v1 resources aren't they?

* master: Add simple makefile and update readme (#142)

warrenbailey · 2018-02-16T11:40:21Z

Approved as tech debt tickets have been raised to fix the other comments and the understanding they'll be address asap in a separate PR.

insacuri

Looks reasonable and tech debt issues have been raised so I'm happy for this to go in.

Issues raised on separate tickets

* master: Feature/support multiple internal users (#140) # Conflicts: # secure_message/api_mocks/internal_user_service_mock.py # secure_message/application.py # secure_message/repository/retriever.py # secure_message/resources/drafts.py # secure_message/validation/domain.py # tests/app/test_app.py # tests/app/test_retriever.py # tests/behavioural/features/draft_post.feature # tests/behavioural/features/drafts_get.feature # tests/behavioural/features/message_post.feature # tests/behavioural/features/messages_get.feature # tests/behavioural/features/steps/from_field.py # tests/behavioural/features/steps/secure_messaging_context_helper.py

Andrew Torrance added 7 commits February 6, 2018 11:27

initial commit - wip - for visibility by other devs. tests not passing

019f5cf

more wip to share with other devs

9561043

wip

584d528

existing tests passing with support for internal user searches not us…

2c74355

…ing user id . I.e a prerequisite to having multiple internal users

Removed unused versioning code

9ed911a

removed unused versioning code

9bb9a02

Merge with Master

f11c500

AndrewTorrance requested review from insacuri, JamesGardiner, warrenbailey, atrozzo and gavinleeedwards February 8, 2018 15:30

dcdarrell9 and others added 2 commits February 8, 2018 15:41

fix merge conflicts

64a34b2

fixing inadvertent change from 'database' to 'repository' in strings

c53446f

atrozzo reviewed Feb 12, 2018

View reviewed changes

rhiGriff reviewed Feb 12, 2018

View reviewed changes

warrenbailey requested a review from ajmaddaford February 12, 2018 16:42

JamesGardiner suggested changes Feb 12, 2018

View reviewed changes

warrenbailey previously requested changes Feb 12, 2018

View reviewed changes

changes for pr comments

f08afb4

warrenbailey reviewed Feb 13, 2018

View reviewed changes

JamesGardiner previously requested changes Feb 13, 2018

View reviewed changes

warrenbailey suggested changes Feb 14, 2018

View reviewed changes

Merge branch 'master' into feature/support-multiple-internal-users

5f8f449

* master: Add simple makefile and update readme (#142)

warrenbailey approved these changes Feb 16, 2018

View reviewed changes

insacuri approved these changes Feb 16, 2018

View reviewed changes

AndrewTorrance merged commit 08470f4 into master Feb 19, 2018

dcdarrell9 deleted the feature/support-multiple-internal-users branch February 28, 2018 09:25



		def set_v1_resources(api):
		api.add_resource(Health, '/health')

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

		return True, result

Feature/support multiple internal users #140

Feature/support multiple internal users #140

Conversation

AndrewTorrance commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesGardiner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesGardiner Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesGardiner Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warrenbailey commented Feb 16, 2018

insacuri left a comment

Choose a reason for hiding this comment

JamesGardiner Feb 13, 2018 •

edited

Loading

JamesGardiner Feb 12, 2018 •

edited

Loading