OpenConceptLab/ocl_issues#2485 | API to get project configurations#862
OpenConceptLab/ocl_issues#2485 | API to get project configurations#862snyaggarwal wants to merge 1 commit intomasterfrom
Conversation
paynejd
left a comment
There was a problem hiding this comment.
Solid foundation — the inheritance refactor is the right move and the new endpoint slots cleanly into existing URL/view patterns. A few items to address before merge, mostly around making the "what is a configuration" rule explicit and tightening the test coverage to actually enforce the ticket's scope contract.
Define CONFIGURATION_FIELDS as a class constant on the MapProject model
(Leaving this at the top level since core/map_projects/models.py isn't in this PR's diff.)
Right now "what counts as a project configuration" lives only as a hardcoded field list in MapProjectConfigurationsSerializer. There's no single source of truth — the model is flat, with config fields (algorithms, filters, lookup_config, …) interleaved with result/runtime fields (matches, candidates, analysis, …) and identity/audit fields. Whether a field is "configuration" is a human judgment that's currently undocumented.
Please add a class-level constant on MapProject, e.g.:
class MapProject(BaseModel):
...
CONFIGURATION_FIELDS = [
'algorithms', 'encoder_model', 'filters', 'include_retired',
'lookup_config', 'score_configuration', 'target_repo_url',
]Then have MapProjectConfigurationsSerializer.Meta.fields reference it (e.g. ['id', 'url'] + MapProject.CONFIGURATION_FIELDS). This:
- Puts the definition next to the field declarations, where future contributors will see it when adding a new field.
- Makes it reusable from
MapProjectCreateUpdateSerializerand any future copy-related logic, so a new config field automatically flows everywhere instead of needing four serializers updated by hand. - Documents intent — anyone reading the model can see the configuration boundary without reverse-engineering it from one of five serializers.
|
|
||
|
|
||
| class MapProjectSerializer(serializers.ModelSerializer): | ||
| class MapProjectConfigurationsSerializer(serializers.ModelSerializer): |
There was a problem hiding this comment.
Once CONFIGURATION_FIELDS lives on the model (see top-level review comment), replace the hardcoded list here with ['id', 'url'] + MapProject.CONFIGURATION_FIELDS. Also worth a one-line docstring on the class stating the rule (e.g. "Fields that define how a project matches — excluding identity, results, logs, and audit metadata. Used by the copy-project flow.") so the contract is discoverable.
Two field-scope questions worth confirming explicitly (in code or PR description):
descriptionis inMapProjectCreateUpdateSerializerbut excluded here — copy won't carry the description forward. Intentional? (Arguably correct since the description likely refers to the original project, but worth being deliberate.)public_accessis in the base serializer but excluded here — the copied project gets default access rather than inheriting. Likely correct, but same point: worth being deliberate.
| key=f"map_projects/{response.data['id']}/input.csv", file_content=ANY) | ||
|
|
||
|
|
||
| class MapProjectConfigurationsViewTest(MapProjectAbstractViewTest): |
There was a problem hiding this comment.
Test asserts that included fields are correct, but doesn't assert the ticket's actual contract: that excluded fields are absent. Please add assertions that the response does NOT contain candidates, logs, summary, analysis, input_file_name, matches, columns. That's what guarantees we're not leaking project identity/results into a copy.
Also missing:
- A negative auth test (unauthorized user → 403/404). Other map-project views are protected by
CanEditConceptDictionary; one assertion confirming this endpoint inherits that protection would lock it down. - A 404 test for a non-existent project ID.
Linked Issue
Ref OpenConceptLab/ocl_issues#2485