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: fix conversation refresh/syncing #937

Merged
merged 5 commits into from
Mar 17, 2020
Merged

app: fix conversation refresh/syncing #937

merged 5 commits into from
Mar 17, 2020

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented Mar 13, 2020

Description

Fixes #891.
Fixes #939.

Test Plan

Submission deleted via JI

  1. Login to client, click on a source
  2. Delete a single submission via the JI for that source
  3. You should see the submission be deleted from the conversation view after sync
  4. For good measure, try sending a reply from the JI and ensure it appears in the conversation view

File deleted via JI

Precondition: Submit a file as a source

  1. Login to client, click on a source
  2. Delete a single file via the JI for that source
  3. You should see the file be deleted from the conversation view after sync
  4. Now submit another file as a source
  5. You should see the new file appear in the conversation view after sync
  6. For good measure, try sending a reply from the JI and ensure it appears in the conversation view
  7. For good measure, try sending a message from the source and ensure it appears in the conversation view

You should not see any of the behavior reported in freedomofpress/securedrop-workstation#494 (comment): the application should not crash with a SQLAlchemy exception, nor should there be any overlaid file widgets in the conversation view.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

kushaldas
kushaldas previously approved these changes Mar 16, 2020
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

Submission deleted via JI

  • Login to client, click on a source
  • Delete a single submission via the JI for that source
  • You should see the submission be deleted from the conversation view after sync
  • For good measure, try sending a reply from the JI and ensure it appears in the conversation view

File deleted via JI

Precondition: Submit a file as a source

  • Login to client, click on a source
  • Delete a single file via the JI for that source
  • You should see the file be deleted from the conversation view after sync
  • Now submit another file as a source
  • You should see the new file appear in the conversation view after sync
  • For good measure, try sending a reply from the JI and ensure it appears in the conversation view
  • For good measure, try sending a message from the source and ensure it appears in the conversation view

The data directory also looks good, no old files on the disk. Needs a rebase.

@redshiftzero
Copy link
Contributor Author

thanks for review! rebased/retested/repushed on latest master (4f86a61)

when we access the replies, messages, and files relationships
on Source, a new query is not necessarily emitted by sqlalchemy.
This means that we can access objects that have been deleted.
To ensure that this is not the case, we can call expire_all on
the session.
@redshiftzero
Copy link
Contributor Author

rebased/retested/repushed on latest master (8ab6101)

Suggested by @rmol in private conversation to replace the
expire_all
@redshiftzero
Copy link
Contributor Author

added a commit based on a comment from @rmol (and retested)

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

Rereviewed, works as required. Approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants