Fix starring behavior

[sssoleileraaa]

Description

WIP until I update unit tests and write a test plan.

Fixes #946
Resolves #760
Closes #811
Fixes #954

This fixes the crash reported in #946, resolves #760 and part of #954 by no longer updating sources whenever a source is starred, closes #811 by retrying star jobs that fail due to timeout, and fixes the rest of #954 so that stars are updated immediately and only toggle back at the same time an error is displayed that says "Failed to update star" or when a star is updated on the server. This also removed the extra callback that toggled stars unpredictably given that there were two callbacks.

Test Plan

1. Make sure this fixes the crash reported in accessing the source db object when starring after deletion crashes client #946
2. To test Standardize connection errors to "Trying to reconnect" language without Retry #811, cause a timeout when starring a source and see the standard "Trying to reconnect" message
3. Test a non-timeout error and see the star toggle back to its original state before the failure along with the error message "Failed to update star"
4. To test the rest of the starring behavior, see Stars changing state unexpectedly #954 and how each of the 4 issues has been fixed. Remember to test in Qubes, also while comparing this PR branch with master

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

[rmol]

1. ✔️ Make sure this fixes the crash reported in accessing the source db object when starring after deletion crashes client #946
2. ✔️ To test Standardize connection errors to "Trying to reconnect" language without Retry #811, cause a timeout when starring a source and see the standard "Trying to reconnect" message
3. ❌ Test a non-timeout error and see the star toggle back to its original state before the failure along with the error message "Failed to update star"

I added raise Exception('boom') at the start of the add_star endpoint, and the resulting 500 crashed the client:

2020-03-20 17:45:22,732 - urllib3.connectionpool:393(_make_request) DEBUG: http://localhost:8081 "POST /api/v1/sources/16f76861-9d7b-431b-9198-2013f9ed0609/add_star HTTP/1.1" 500 None
2020-03-20 17:45:22,751 - root:53(excepthook) ERROR: Unrecoverable error
Traceback (most recent call last):
  File "/home/user/src/fpf/securedrop-client/securedrop_client/logic.py", line 525, in on_update_star_failure
    self.star_update_failed.emit(error.source_uuid, error.is_starred)
TypeError: Controller.star_update_failed[str] signal has 1 argument(s) but 2 provided

4. To test the rest of the starring behavior, see Stars changing state unexpectedly #954 and how each of the 4 issues has been fixed.

• Issue 1: ❌ I made the journalist interface unresponsive with kill -STOP <its pid> (could have just added a sleep to add_star instead, I guess) and got the The SecureDrop server cannot be reached message, but the star remained checked, as the client kept sending jobs to update it.
• Issues 2 and 3: ✔️ Syncs didn't undo the checking of stars.
• Issue 4: ✔️ I didn't see any timeouts.

[sssoleileraaa]

So I was able to fix # 3 and # 4 Issue 1 in the last commit. You should see the star go back to the current source.is_starred state after a non-timeout error at the same time you see the "Failed to update star" message in the error bar.

[sssoleileraaa]

oops, so first i didn't push the commit, and now it has new conflicts with the latest changes on master... i'll fix now...

[sssoleileraaa]

the conflict was in set_snippet and i just accepted the new set_snippet code on master and resigned all my commits. also just want to mention this is how i tested # 3 and # 4 issue 1:

1. apply this diff:

--- a/securedrop_client/api_jobs/updatestar.py
+++ b/securedrop_client/api_jobs/updatestar.py
@@ -22,6 +22,9 @@ class UpdateStarJob(ApiJob):
         Star or Unstar an user on the server
         '''
         try:
+            import time
+            time.sleep(3)
+            raise Exception('errrrororror')
             source_sdk_object = sdclientapi.Source(uuid=self.source_uuid)
 
             # TODO: Once https://github.com/freedomofpress/securedrop-client/issues/648, we will

2. log into the client
3. toggle a button once from off to on and wait to see the error message and it go back to the correct state (source.is_starred)
4. toggle a button once from on to off and wait to see the error message and it go back to the correct state (source.is_starred)
5. toggle a button twice and wait to see the error message and it remain in the same correct state.
   etc. until you're convinced

[rmol]

I switched to your method of breaking the star update job, but I'm still not seeing the stars reset to the state before they were toggled. ☹️

[sssoleileraaa]

i just tested in qubes and am seeing this issue resolved so one of us is probably forgetting to do something - probably me. but it's late on friday so maybe we could both retest on monday and figure out why we see different things?

[rmol]

It's the sleep you have in your test breakage. If I add that, the star does get reset. Without it, it does not; it's left starred.

[rmol]

This looks good now; scheduling the update works for exceptions in client or server, starring or unstarring. If you're happy, I think it's ready to merge.

[sssoleileraaa]

(rebased)

[rmol]

👍