Skip to content

fix: ignore non-JSON project share responses#675

Open
gaoflow wants to merge 2 commits into
TimMcCool:mainfrom
gaoflow:fix-609-project-share-non-json-response
Open

fix: ignore non-JSON project share responses#675
gaoflow wants to merge 2 commits into
TimMcCool:mainfrom
gaoflow:fix-609-project-share-non-json-response

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 27, 2026

Copy link
Copy Markdown

Fixes #609.

Summary

  • wrap Project.share() in requests.no_error_handling() so a successful Scratch share response with a non-JSON body does not raise from the shared response checker
  • add a regression test that asserts share() performs the PUT while response error handling is disabled, then restores the global handler flag

Verification

  • uv run --project tests pytest tests/test_project.py::test_project_share_disables_response_json_error_handling -q
  • uv run --project tests pytest tests/test_project.py -q
  • uv run --with ruff ruff check scratchattach/site/project.py tests/test_project.py
  • git diff --check

Note: tests/test_project.py reports the existing credentials-dependent integration test as skipped via warning when local Scratch credentials are unavailable.

LLM disclosure

Codex was used to inspect the issue, make the code/test change, run verification commands, and draft this PR description. The final patch was reviewed through local tests and git diff before submission.

@faretek1

Copy link
Copy Markdown
Collaborator

Overcomplicated tests. Is this llm generated?

Drop the brittle full-call assertion (exact args/headers dict) and the
module-level FakeSession in favor of asserting only the invariant the fix
guarantees: share() runs the PUT with error_handling disabled and restores
it afterwards.
@gaoflow

gaoflow commented Jun 29, 2026

Copy link
Copy Markdown
Author

Good call — simplified it. The test now just asserts the invariant the fix guarantees (share() runs the PUT with error_handling disabled and restores it afterward) and drops the brittle exact-arg/header comparison. Happy to trim it further if you'd prefer.

@gaoflow

gaoflow commented Jun 29, 2026

Copy link
Copy Markdown
Author

To answer your question directly: yes, I use AI assistance when drafting, under my own direction and review. The #609 bug is real, and I verified both the fix and the now-simplified test pass locally. Happy to adjust the implementation however you'd prefer.

@faretek1

Copy link
Copy Markdown
Collaborator

Hmm, i have checked the code a bit more and it seems like the reason why the tests are "overcomplicated" is because essentially it is doing interception. I still think this could be optimised however I can appreciate that this test would be different because if we tested it directly with the scratch website, it would cause an action (which mutates the state of the scratch website). However this is just a drive-by PR caused by the good first issue tag (again). Maybe the idea can be taken (and improved on).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: project.share

2 participants