fix: Handle case of disconnected submissions

[prateekj117]

Status

Ready for Review

Description of Changes

Fixes #5315 .

Changes proposed in this pull request:

Handled the case of disconnected submissions and added a test for the same.

Checklist

If you made changes to the server application code:

• Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

• Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

• Configuration tests pass

If you made non-trivial code changes:

• I have written a test plan and validated it for this PR

If you made changes to documentation:

• Doc linting (make docs-lint) passed locally

[prateekj117]

@eloquence Found some issues. Not ready for review. 😓

[prateekj117]

@rmol @eloquence @redshiftzero So sqlalchemy does a cascade delete, on the other hand, directly running the query delete from sources where id=1; doesn't. So this test is not the right way to check this code. Can you guide me on how I should go about writing the test here?

[rmol]

You could use that query in the test instead of deleting the source in the usual way:

        # db.session.delete(test_submissions['source'])
        db.session.execute(
            "DELETE FROM sources WHERE id = :id",
            {"id": test_submissions["source"].id}
        )

[prateekj117]

@rmol Oh yes. Ofc. Thanks.

[prateekj117]

@rmol Done. Ready for review. Tested the test on local. Didn't face the earlier issue. Works fine now. 👍
@eloquence Ready for review. 😄

[rmol]

This looks good. The test works. I've gone through the STR and verified that the Submission.to_json changes solve the 500.

I had a bit of last-minute indecision about whether the get_all_submissions API endpoint should even be returning disconnected submissions, instead of omitting those without a source, but the very name suggests it should. I checked the client code and it should properly deal with downloaded submissions which don't have a source -- if a new submission has no source, it is simply not added, and if the submission exists locally (which should only happen if the local source hasn't been deleted during a sync yet) it will be updated.

[prateekj117]

@rmol mmmm, ok. Ya sounds right not to return them. Let me know if we need to do that and I will edit the PR according to that.

discovered problems

[rmol]

Yeah, when actually running the client I discovered that its handling of disconnected submissions is fine, but the SDK throws an exception when the source doesn't exist. And there are similar problems with the get_all_replies endpoint, for which I'll file a separate issue.

For this PR, adding a check of the source to api.get_all_submissions will exclude the disconnects and avoid sending problematic data to the SDK and client:

        return jsonify({'submissions': [s.to_json() for
                                        s in submissions if s.source]}), 200

Thanks for persevering, @prateekj117!

[prateekj117]

@rmol Sure. Will make those changes.

[prateekj117]

@rmol Done. Ready for another review.

[rmol]

Thanks! I was hoping to just add the filtering of source-less submissions in get_all_submissions while keeping the improvements to Submission.to_json -- could you restore that change to this final version? I don't want to deter you, so I can add it if you'd prefer to be done with this.

[prateekj117]

@rmol Not an issue. Will do that change.

[prateekj117]

@rmol Done.

[eloquence]

(CI failure likely unrelated, kicking)

[rmol]

👍 Looks great, thanks for all your work on this @prateekj117.