Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for SubjectSet metadata as editable attribute #303

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lcjohnso
Copy link
Member

Closes zooniverse/panoptes-cli#256

The issue: when SubjectSet.metadata attribute is edited, there is no check for changes in that field, so it is never added to modified_attributes set and therefore those edits were never pushed as a result of the save() call.

Following same solution as in #222 and #251 for workflow and project attributes:

  • add code to initialize current vs original check of metadata dict in set_raw and __init__ functions
  • add check to save() to add metadata attribute to modified_attributes if contents are different

Note on change in treatment of SubjectSet.metadata field: I've changed metadata to a standalone key in SubjectSet._edit_attributes field as opposed to dict similar to links field. The distinction between these two treatments of the attribute occur in PanoptesObject._savable_dict() (see https://github.com/zooniverse/panoptes-python-client/blob/master/panoptes_client/panoptes.py#L759-L793), but I'm following the precedent already used by Subject class and elsewhere for these dict attribute fields.

Note on CLI issue: although treatment of SubjectSet metadata has been implemented in the CLI for ~2 years (since zooniverse/panoptes-cli#214), it looks like this feature -- identifying and flagging indexed fields by adding them to indexFields key in SubjectSet metadata -- has never worked up to this point.

This PR currently needs testing:

  • check that metadata edits behave as expected
  • check that CLI issue is in fact resolved

@eatyourgreens
Copy link

Subject set tests need updating somewhere to add in metadata.

File "/home/runner/work/panoptes-python-client/panoptes-python-client/panoptes_client/tests/test_subject_set.py", line 31, in test_create
    pc.client().post.assert_called_with(
  File "/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/unittest/mock.py", line 907, in assert_called_with
    raise AssertionError(_error_message()) from cause
AssertionError: expected call not found.
Expected: post('/subject_sets', json={'subject_sets': {'display_name': 'Name', 'links': {'project': 1234}}}, etag=None)
Actual: post('/subject_sets', json={'subject_sets': {'display_name': 'Name', 'metadata': {}, 'links': {'project': 1234}}}, etag=None)

@snblickhan
Copy link

Commenting so I can get emails about progress of this PR (not a rush, just so I don't forget)

@lcjohnso
Copy link
Member Author

lcjohnso commented Jun 3, 2024

This PR currently has an implementation issue: most SubjectSets do not have any metadata, so therefore metadata == {}. When the SubjectSet object is initialized and set_raw() runs, that means self.metadata will evaluate as False and the elif statement will run (see https://github.com/zooniverse/panoptes-python-client/pull/303/files#diff-26c3a9c285248c88ab6554abda7a5e745a416b746451a546640287adced90766R79-R80) and set _original_metadata to None, causing problems when that variable needs to be evaluated in SubjectSet.save() to check for metadata changes.

This buggy behavior also exists for subjects (and elsewhere), but hasn't been discovered until now because most other resources have non-empty metadata so never encounter this case.

The simple solution would be to always set _original_metadata via deepcopy() as in the current L78 of set_raw(). That seems to work for an empty dict, but perhaps I'm missing why a separate statement for self.metadata = False was originally added. Some testing would be good to do here.

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.

A manifest with %header headers doesn't create an indexed subject set
4 participants