Support all annotation types in add_annotations API (#7999)

[philipkukulak]

Proposed Changes

The API endpoint POST add_annotations now supports all annotation types. Co-authored with Claude. Closes #7999.

In achieving this, it was found that there was no validation of annotation type to file type for which we are seeking to annotate. This means a request to apply a PdfAnnotation to a .py file would succeed, which would break rendering in the UI. To resolve this, some pre-existing type derivation logic was lifted out of the UI AnnotationsController and is now being used in both the UI and API controllers; the API also uses it as a part of the validation chain.

As a side effect of this change, add_existing_annotation's specs were lacking coverage due to a mix in record creation between annotation types: HtmlAnnotation used create!, and all other annotations used create. This is now consolidated. There was no coverage of the failure path for this record creation, so this was added here for good measure.

Another small bug that was fixed is that prior to this change, POSTing a TextAnnotation to a non-text file could create an annotation that would never be displayed on that file. Users will now see a 422 response if they try to do this.

The API endpoint itself now either accepts an explicit annotation type, or derives it implicitly. In the former case, it expects a match between the annotation type and the file being annotated. In the latter, it simply reconciles it from the file being annotated.

Associated documentation repository pull request (if applicable)

MarkUsProject/Wiki#267

Type of Change

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	x
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

• I have updated the project Changelog (this is required for all changes).
• I have verified that the pre-commit.ci checks have passed.
• I have verified that the CI tests have passed.
• I have reviewed the test coverage changes reported by Coveralls.
• I have requested a review from a project maintainer.

Questions and Comments

Here's a summary of the smoke tests for the changes to the endpoint:

Smoke test — add_annotations API

Local server (/csc108), authenticated instructor API key. Seeded one file per type on a
version-used submission: hello.py, nemo.jpg, possimus.pdf, ipsa.ipynb. Each result
confirmed by reading back through GET …/groups/:id/annotations. The type field uses the
full STI class names (TextAnnotation/ImageAnnotation/PdfAnnotation/HtmlAnnotation).

#	Scenario	type sent	Target file	Expected	Result
1	Text annotation	TextAnnotation	hello.py	200, TextAnnotation	✅ 200 — TextAnnotation, line/col persisted
2	Image annotation	ImageAnnotation	nemo.jpg	200, ImageAnnotation	✅ 200 — ImageAnnotation, x/y persisted
3	PDF annotation	PdfAnnotation	possimus.pdf	200, PdfAnnotation	✅ 200 — PdfAnnotation, page 2
4	HTML annotation	HtmlAnnotation	ipsa.ipynb	200, HtmlAnnotation	✅ 200 — HtmlAnnotation, nodes persisted
5	Type omitted → derived from file	(none)	possimus.pdf	200, PdfAnnotation	✅ 200 — PdfAnnotation, page 3
6	Type ↔ file mismatch	TextAnnotation	possimus.pdf	422	✅ 422 — Annotation type 'TextAnnotation' does not match the type of file 'possimus.pdf'
7	Unknown / invalid type	bogus	hello.py	422	✅ 422 — Invalid annotation type: bogus
8	Missing required field	(derived pdf)	possimus.pdf (no page)	422	✅ 422 — Missing required fields for PdfAnnotation: page
9	Nonexistent file	TextAnnotation	nope.txt	422	✅ 422 — Submission file not found: nope.txt
10	Batch atomicity (1 valid + 1 invalid)	TextAnnotation, TextAnnotation	hello.py, possimus.pdf	422, nothing written	✅ 422 — DB count unchanged; valid item not written
11	Empty annotations array	—	—	(edge)	✅ 200 — no-op, nothing written

Net: all 5 valid writes persisted with the correct STI type on the correct file; all 4
bad-input paths failed closed with specific 422 messages and left the DB unchanged; batch
validation is atomic (rejects the whole request before any insert).

Note: type accepts the STI class names; a short code such as "text" is rejected as
Invalid annotation type: text (consistent with the add_test_results endpoint).

Note: API error description values are HTML-escaped (e.g. ' for ') by the shared
shared/http_status.json.erb renderer — pre-existing, app-wide behavior, not introduced by
this change.

[coveralls]

Coverage Report for CI Build 27791495213

Coverage increased (+0.02%) to 90.24%

Details

• Coverage increased (+0.02%) from the base build.
• Patch coverage: 117 of 117 lines across 6 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	50430
Covered Lines:	46498
Line Coverage:	92.2%
Relevant Branches:	2284
Covered Branches:	1071
Branch Coverage:	46.89%
Branches in Coverage %:	Yes
Coverage Strength:	126.18 hits per line

---

💛 - Coveralls

[david-yz-liu]

Okay, so I think defining this is good, but really this information belongs in each model. It's a bit awkward to translate from string name to class, but since we're always deriving the expected class from the submission file anyway, I suggest the following changes:

1. Modify SubmissionFile.annotation_type to be SubmissionFile.annotation_class and return the class itself (e.g., TextAnnotation) rather than the string ("TextAnnotation").
2. You can go from the class to its name with the .name method
3. Define the symbol lists in each of the Annotation subclasses and then access them instead of REQUIRED_ANNOTATION_FIELDS[expected_type] below
4. There's a use of REQUIRED_ANNOTATION_FIELDS.key? below, but I don't think we need it. It's sufficient to check against the expect type and report the "does not match the type" error, without having a separate "Invalid annotation type" error.

[david-yz-liu]

A separate pass isn't necessary here. This check can happen inside the existing loop below; the early return will ensure nothing is inserted into the database, as that only happens after the loop

[david-yz-liu]

Let's be stricter here.

1. If there are any required fields that are missing, report an error (this is what the previous params.require call would have done)
2. If there are additional annotation fields that are "for the wrong type" (e.g. passing line_start for a PdfAnnotation) also report an error and return

Then, extract the annotation attributes (you can use the Ruby Hash#slice method and use that to create the hash that's appended to annotations, rather than passing all attributes.

[david-yz-liu]

I'd like to keep the require, which was strict with the param requirement. I assume you changed this to allow the inner hashes to not have all of the attributes.

I think you can achieve both the strict :annotations key and the permissive inner structure by using the new expect method

[david-yz-liu]

Right, so this is the part where I'd like the logic to be the same for both controller methods.

1. Get the list of symbols from a model method
2. Call Hash#slice to get the values from params (no need to define a separate method)