Added the ability to request a removal of a specified submission to the CLI

[OliverLok]

No description provided.

[eriq-augustine]

A good start, and needs some work.

Be careful, I felt like this one was a bit more loose than the server one.
Don't let your guard down on Python.

We also need some tests.

[eriq-augustine]

The code here looks good!
Only comment on the existing code is that you should rename:
"remove-submission" -> "remove".
Since it is already in the "submission" package, the prefix is redundant.

The only remaining thing here is tests.

API tests are done with JSON data:
https://github.com/eriq-augustine/autograder-py/tree/main/tests/api/data
The testing infrastructure knows how to look through the data, find the tests, call the right API method, and check the results.
The easiest way to get the expected output for the tests is to run the CLI call on the Python side (e.g. python3 -m autograder.cli.submission.remove --verbose ...).
With the verbose flag, the exact request and response will be printed out.

You can see examples with peek:
https://github.com/eriq-augustine/autograder-py/blob/main/tests/api/data/test_submission_peek_other_specific.json

Note that the data for API tests will actually be validated later (automatically) in the CI for the server.

[OliverLok]

The code here looks good! Only comment on the existing code is that you should rename: "remove-submission" -> "remove". Since it is already in the "submission" package, the prefix is redundant.

The only remaining thing here is tests.

API tests are done with JSON data: https://github.com/eriq-augustine/autograder-py/tree/main/tests/api/data The testing infrastructure knows how to look through the data, find the tests, call the right API method, and check the results. The easiest way to get the expected output for the tests is to run the CLI call on the Python side (e.g. python3 -m autograder.cli.submission.remove --verbose ...). With the verbose flag, the exact request and response will be printed out.

You can see examples with peek: https://github.com/eriq-augustine/autograder-py/blob/main/tests/api/data/test_submission_peek_other_specific.json

Note that the data for API tests will actually be validated later (automatically) in the CI for the server.

With that logic, should "removesubmission" in api/submission also be renamed to remove?
Also, just to clarify, is the CI sending requests and comparing the request/response JSON to the corresponding json file in tests/api/data? I'm only asking because when I changed found-user to true in test_submission_peek_other_missing.json (where the target-email was "ZZZ"), ./test.sh still said No issues found.

[eriq-augustine]

With that logic, should "removesubmission" in api/submission also be renamed to remove?

Yes.

Also, just to clarify, is the CI sending requests and comparing the request/response JSON to the corresponding json file in tests/api/data?

The API tests (which is run in the CI) is sending itself HTTP requests and checking that they match up against it's own expected output (the same file is used to specify the input an output):
https://github.com/eriq-augustine/autograder-py/blob/main/tests/api/test_api.py#L28

I'm only asking because when I changed found-user to true in test_submission_peek_other_missing.json (where the target-email was "ZZZ"), ./test.sh still said No issues found.

https://github.com/eriq-augustine/autograder-py/blob/main/tests/api/test_api.py#L32
On this side, the data is expected to be correct.
So when you changed the data, you changed what the test thought was correct.
But the server will verify the data in it's own CI:
https://github.com/eriq-augustine/autograder-server/blob/main/.ci/verify-py-test-data.sh

[eriq-augustine]

Test content is looking good.
Just need to get our gitignore back and adjust the spacing in the tests.

[eriq-augustine]

LGTM

We are just waiting on the sibling PR in the server:
edulinq/autograder-server#39

[eriq-augustine]

Looks like I spoke too soon, CI is failing.
Did you run tests locally?

[eriq-augustine]

One of the test cases is incorrect and should not be passing.
This is something that should be caught by the standard tests.
If this is not getting caught on your local test runs, it is a pretty big issue.

[eriq-augustine]

LGTM
We are now just waiting on the server PR.