update reply sender name during sync if it has changed

requirements/build-requirements.txt

@@ -9,7 +9,7 @@ pathlib2==2.3.2 --hash=sha256:460e67b14d0574b0529a0017b1eb05d10d9722681e303fec70
 python-dateutil==2.7.5 --hash=sha256:f6eb9c17acd5a6954e1a5f2f999a41de3e7e25b6bc41baf6344bd053ec25ceeb
 python-editor==1.0.3 --hash=sha256:e47dcec4ea883853b8196fbd425b875d7ec791d4ede2e20cfc70b9a25365c65b
 requests==2.20.0 --hash=sha256:d87b2085783d31d874ac7bc62660e287932aaee7059e80b41b76462eb18d35cc
-securedrop-sdk==0.1.0 --hash=sha256:488417f9f08e4c432c81348dfbd5da0e756ded1737ba58b2ffc8f0e703abc1cb
+securedrop-sdk==0.1.1 --hash=sha256:a631495acd741ab568410287879c5a3af3ccd38e00a2f3a127cc6b27cba99392
 six==1.11.0 --hash=sha256:aa4ad34049ddff178b533062797fd1db9f0038b7c5c2461a7cde2244300b9f3d
 sqlalchemy==1.3.3 --hash=sha256:bfb4cd0df5802a01acd738a080a19e60ff4700e030d497de273807f9a8bd4a0a
 urllib3==1.24.3 --hash=sha256:3d440cbb168e2c963d5099232bdb3f7390bf031b6270dad1bc79751698a1399a

requirements/dev-requirements.txt

@@ -377,8 +377,8 @@ requests==2.20.0 \
     --hash=sha256:99dcfdaaeb17caf6e526f32b6a7b780461512ab3f1d992187801694cba42770c \
     --hash=sha256:a84b8c9ab6239b578f22d1c21d51b696dcfe004032bb80ea832398d6909d7279 \
     # via -r requirements/requirements.in, securedrop-sdk
-securedrop-sdk==0.1.0 \
-    --hash=sha256:970fde25e6238e1808ac120951ee972549f4cd7952966dfe29f731bb308cc0d8 \
+securedrop-sdk==0.1.1 \
+    --hash=sha256:138ce7a717db519c3c8d19b9475d7660fb7095d7608e8802723e682a7415e677 \
     # via -r requirements/requirements.in
 sip==4.19.8 \
     --hash=sha256:09f9a4e6c28afd0bafedb26ffba43375b97fe7207bd1a0d3513f79b7d168b331 \

requirements/requirements.in

@@ -9,7 +9,7 @@ pathlib2==2.3.2
 python-dateutil==2.7.5
 python-editor==1.0.3
 requests==2.20.0
-securedrop-sdk==0.1.0
+securedrop-sdk==0.1.1
 six==1.11.0
 SQLAlchemy==1.3.3
 urllib3==1.24.3

requirements/requirements.txt

@@ -75,8 +75,8 @@ requests==2.20.0 \
     --hash=sha256:99dcfdaaeb17caf6e526f32b6a7b780461512ab3f1d992187801694cba42770c \
     --hash=sha256:a84b8c9ab6239b578f22d1c21d51b696dcfe004032bb80ea832398d6909d7279 \
     # via -r requirements/requirements.in, securedrop-sdk
-securedrop-sdk==0.1.0 \
-    --hash=sha256:970fde25e6238e1808ac120951ee972549f4cd7952966dfe29f731bb308cc0d8 \
+securedrop-sdk==0.1.1 \
+    --hash=sha256:138ce7a717db519c3c8d19b9475d7660fb7095d7608e8802723e682a7415e677 \
     # via -r requirements/requirements.in
 six==1.11.0 \
     --hash=sha256:70e8a77beed4562e7f14fe23a786b54f6296e34344c23bc42f07b15018ff98e9 \

securedrop_client/api_jobs/sync.py

@@ -5,7 +5,7 @@
 from sqlalchemy.orm.session import Session
 
 from securedrop_client.api_jobs.base import ApiJob
-from securedrop_client.storage import get_remote_data, update_and_get_user, update_local_storage
+from securedrop_client.storage import create_or_update_user, get_remote_data, update_local_storage
 
 logger = logging.getLogger(__name__)
 
@@ -42,6 +42,6 @@ def call_api(self, api_client: API, session: Session) -> Any:
         update_local_storage(session, sources, submissions, replies, self.data_dir)
         user = api_client.get_current_user()
         if "uuid" in user and "username" in user and "first_name" in user and "last_name" in user:
-            update_and_get_user(
-                user["uuid"], user["username"], user["first_name"], user["last_name"], session,
+            create_or_update_user(
+                user["uuid"], user["username"], user["first_name"], user["last_name"], session
             )

securedrop_client/logic.py

@@ -457,11 +457,11 @@ def on_authenticate_success(self, result):
         """
         logger.info("{} successfully logged in".format(self.api.username))
         self.gui.hide_login()
-        user = storage.update_and_get_user(
+        user = storage.create_or_update_user(
             self.api.token_journalist_uuid,
             self.api.username,
-            self.api.journalist_first_name,
-            self.api.journalist_last_name,
+            self.api.first_name,
+            self.api.last_name,
             self.session,
         )
         # Clear clipboard contents in case of previously pasted creds

securedrop_client/storage.py

@@ -299,8 +299,28 @@ def update_replies(
     source_cache = SourceCache(session)
     for reply in remote_replies:
         user = users.get(reply.journalist_uuid)
+
         if not user:
-            user = find_or_create_user(reply.journalist_uuid, reply.journalist_username, session)
+            user = create_or_update_user(
+                reply.journalist_uuid,
+                reply.journalist_username,
+                reply.journalist_first_name,
+                reply.journalist_last_name,
+                session,
+            )
+            users[reply.journalist_uuid] = user
+        elif (
+            user.username != reply.journalist_username
+            or user.firstname != reply.journalist_first_name
+            or user.lastname != reply.journalist_last_name
+        ):
+            user = create_or_update_user(
+                reply.journalist_uuid,
+                reply.journalist_username,
+                reply.journalist_first_name,
+                reply.journalist_last_name,
+                session,
+            )
             users[reply.journalist_uuid] = user
 
         local_reply = local_replies_by_uuid.get(reply.uuid)
@@ -357,7 +377,9 @@ def update_replies(
     session.commit()
 
 
-def find_or_create_user(uuid: str, username: str, session: Session, commit: bool = True) -> User:
+def create_or_update_user(
+    uuid: str, username: str, firstname: str, lastname: str, session: Session
+) -> User:
     """
     Returns a user object representing the referenced journalist UUID.
     If the user does not already exist in the data, a new instance is created.
@@ -366,30 +388,15 @@ def find_or_create_user(uuid: str, username: str, session: Session, commit: bool
     user = session.query(User).filter_by(uuid=uuid).one_or_none()
 
     if not user:
-        new_user = User(username=username)
+        new_user = User(username=username, firstname=firstname, lastname=lastname)
         new_user.uuid = uuid
         session.add(new_user)
-        if commit:
-            session.commit()
+        session.commit()
         return new_user
 
     if user.username != username:
         user.username = username
-        if commit:
-            session.commit()
-
-    return user
-
-
-def update_and_get_user(
-    uuid: str, username: str, firstname: str, lastname: str, session: Session
-) -> User:
-    """
-    Returns a user object representing the referenced journalist UUID.
-    If user fields have changed, the db is updated.
-    """
-    user = find_or_create_user(uuid, username, session)
-
+        session.commit()
     if user.firstname != firstname:
         user.firstname = firstname
         session.commit()

tests/test_logic.py

@@ -219,8 +219,8 @@ def test_Controller_on_authenticate_success(homedir, config, mocker, session_mak
     co.api = mocker.MagicMock()
     co.api.token_journalist_uuid = user.uuid
     co.api.username = user.username
-    co.api.journalist_first_name = user.firstname
-    co.api.journalist_last_name = user.lastname
+    co.api.first_name = user.firstname
+    co.api.last_name = user.lastname
     co.resume_queues = mocker.MagicMock()
 
     co.on_authenticate_success(True)

tests/test_storage.py

@@ -15,12 +15,12 @@
 import securedrop_client.db
 from securedrop_client import db
 from securedrop_client.storage import (
+    create_or_update_user,
     delete_local_source_by_uuid,
     delete_single_submission_or_reply_on_disk,
     find_new_files,
     find_new_messages,
     find_new_replies,
-    find_or_create_user,
     get_file,
     get_local_files,
     get_local_messages,
@@ -35,7 +35,6 @@
     mark_as_not_downloaded,
     set_message_or_reply_content,
     source_exists,
-    update_and_get_user,
     update_draft_replies,
     update_file_size,
     update_files,
@@ -95,6 +94,8 @@ def make_remote_reply(source_uuid, journalist_uuid="testymctestface"):
         filename="1-reply.filename",
         journalist_uuid=journalist_uuid,
         journalist_username="test",
+        journalist_first_name="test",
+        journalist_last_name="test",
         file_counter=1,
         is_deleted_by_source=False,
         reply_url="test",
@@ -683,7 +684,7 @@ def test_update_messages(homedir, mocker):
     local_user.id = 42
     mock_session.query().filter_by().first.return_value = local_source
     mock_focu = mocker.MagicMock(return_value=local_user)
-    mocker.patch("securedrop_client.storage.find_or_create_user", mock_focu)
+    mocker.patch("securedrop_client.storage.create_or_update_user", mock_focu)
     mock_delete_submission_files = mocker.patch(
         "securedrop_client.storage.delete_single_submission_or_reply_on_disk"
     )
@@ -749,8 +750,10 @@ def test_update_replies(homedir, mocker, session):
     # local database, the other will NOT exist in the local database
     # (this will be added to the database)
     remote_reply_update = factory.RemoteReply(
-        journalist_uuid=journalist.uuid,
         uuid=local_reply_update.uuid,
+        journalist_uuid=journalist.uuid,
+        journalist_first_name="new_name",
+        journalist_last_name="new_name",
         source_url="/api/v1/sources/{}".format(source.uuid),
         file_counter=local_reply_update.file_counter,
         filename=local_reply_update.filename,
@@ -761,6 +764,8 @@ def test_update_replies(homedir, mocker, session):
         source_url="/api/v1/sources/{}".format(source.uuid),
         file_counter=factory.REPLY_COUNT + 1,
         filename="{}-filename.gpg".format(factory.REPLY_COUNT + 1),
+        journalist_first_name="",
+        journalist_last_name="",
     )
 
     remote_replies = [remote_reply_update, remote_reply_create]
@@ -867,70 +872,69 @@ def test_update_replies_missing_source(homedir, mocker, session):
     error_logger.assert_called_once_with(f"No source found for reply {remote_reply.uuid}")
 
 
-def test_find_or_create_user_existing_uuid(mocker):
+def test_create_or_update_user_existing_uuid(mocker):
     """
     Return an existing user object with the referenced uuid.
     """
     mock_session = mocker.MagicMock()
     mock_user = mocker.MagicMock()
     mock_user.username = "foobar"
+    mock_user.firstname = "foobar"
+    mock_user.lastname = "foobar"
     mock_session.query().filter_by().one_or_none.return_value = mock_user
-    assert find_or_create_user("uuid", "foobar", mock_session) == mock_user
+    assert create_or_update_user("uuid", "username", "fn", "ln", mock_session) == mock_user
 
 
-def test_find_or_create_user_update_username(mocker, session):
+def test_create_or_update_user_update_username(mocker, session):
     """
     Return an existing user object with the updated username.
     """
     user = factory.User(uuid="mock_uuid", username="mock_old_username")
     session.add(user)
 
-    actual_user = find_or_create_user("mock_uuid", "mock_username", session)
+    actual_user = create_or_update_user("mock_uuid", "mock_username", "fn", "ln", session)
 
     assert actual_user == user
     assert actual_user.username == "mock_username"
+    assert actual_user.firstname == "fn"
+    assert actual_user.lastname == "ln"
 
 
-def test_find_or_create_user_new(mocker):
+def test_create_or_update_user_new(mocker):
     """
     Create and return a user object for an unknown username.
     """
     mock_session = mocker.MagicMock()
     mock_session.query().filter_by().one_or_none.return_value = None
-    new_user = find_or_create_user("uuid", "unknown", mock_session)
+    new_user = create_or_update_user("uuid", "unknown", "unknown", "unknown", mock_session)
     assert new_user.username == "unknown"
     mock_session.add.assert_called_once_with(new_user)
     mock_session.commit.assert_called_once_with()
 
 
-def test_update_and_get_user(mocker, session):
+def test_create_or_update_user(mocker, session):
     """
-    Return an existing user object with the updated username.
+    Return an existing user object with the updated username, firstname, and lastname.
     """
     user = factory.User(
         uuid="mock_uuid",
-        username="mock_username",  # username is needed for find_or_create_user
+        username="mock_old_username",  # username is needed for create_or_update_user
         firstname="mock_old_firstname",
         lastname="mock_old_lastname",
     )
     session.add(user)
-    find_or_create_user_fn = mocker.patch(
-        "securedrop_client.storage.find_or_create_user", return_value=user
-    )
 
-    actual_user = update_and_get_user(
+    updated_user = create_or_update_user(
         uuid="mock_uuid",
         username="mock_username",
         firstname="mock_firstname",
         lastname="mock_lastname",
         session=session,
     )
 
-    find_or_create_user_fn.assert_called_with("mock_uuid", "mock_username", session)
-    assert actual_user == user
-    assert actual_user.username == "mock_username"
-    assert actual_user.firstname == "mock_firstname"
-    assert actual_user.lastname == "mock_lastname"
+    assert updated_user.username == "mock_username"
+    assert updated_user.firstname == "mock_firstname"
+    assert updated_user.lastname == "mock_lastname"
 
 
 def test_find_new_messages(mocker, session):