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

Feature/APIv2 Node Settings [PLAT-924] #8536

Merged

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Jul 11, 2018

Paired with @erinspace

Purpose

Implement a /v2/nodes/{nid}/settings/ endpoint that accepts GET and PATCH requests.

Changes

  • Added node settings endpoint /nodes/<node_id>/settings/
  • Added a NodeSettings and NodeSettingsUpdate serializer
  • Added NodeSettings view
  • Exposed settings relationship on node serializer, hidden on registration serializer
  • Modified is_deprecated to allow specifying just a min or max version
  • Modified HideIfWikiDisabled to hide disabled wiki relationship link on versions > 2.7 (bugfix, needed since we had to add a new version).
  • Added new version 2.9 that deprecates access_requests_enabled on the NodeSerializer

QA Notes

Documentation

CenterForOpenScience/developer.osf.io#22

Side Effects

Ticket

https://openscience.atlassian.net/browse/PLAT-924

pattisdr and others added 12 commits July 10, 2018 11:45
- Url added, view and serializer started.
- New permission class IsContributor added.
- Use this serializer for PUT or PATCH requests
- Update the view to choose the right serializer
…isdr/osf.io into feature/node_settings_apiV2

* 'feature/node_settings_apiV2' of https://github.com/pattisdr/osf.io:
  Add test for view_only_links relationship
  Add tests for the NodeSettingsSerializer and GET requests

# Conflicts:
#	api_tests/nodes/views/test_node_settings.py
… loaded multiple times.

- Version 2.9 deprecates access_requests_enabled field from NodeSerializer and moves it to NodeSettings serializer.
- Wiki and forward addons pushed to serializer context so they are not loaded multiple times in serializer method fields and in update method.
…ified, if desired.

- Modify HideIfWikiDisabled so versions greater than 2.7 don't show wiki relationship on nodes if wiki is disabled. Formerly, this feature was just restricted to 2.8.
- List settings field as field that doesn't appear on registration serializer.
- Add test for deprecated access_requests_enabled field on NodeSerializer
type_ = 'node-settings'


class NodeSettingsUpdateSerializer(NodeSettingsSerializer):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erinspace pointed out writeable_method_fields which allow writes to a SerializerMethodField. We explored this, but ended up opting for an UpdateSerializer for writes, so we could keep our individual field validation. The NodeSettings serializer is tricky because most of the fields do not actually reside on the node.

@pattisdr pattisdr changed the title [WIP] Feature/APIv2 Node Settings [PLAT-924] Feature/APIv2 Node Settings [PLAT-924] Jul 13, 2018
@pattisdr
Copy link
Contributor Author

Local travis passed, this test failure seems unrelated.

@erinspace erinspace force-pushed the feature/node_settings_apiV2 branch from 3a63859 to d74bf6a Compare July 18, 2018 19:47
if not forward_addon:
raise exceptions.ValidationError('You must first set redirect_link_enabled to True before specifying a redirect link URL.')
forward_addon.url = redirect_link_url
obj.add_log(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log is added manually here, much like it is manually added elsewhere in the codebase like here where it's mentioned that logging should be consolidated to a more central place.

Decided to leave it as it was and add the log manually as moving those seemed a bit out of scope, but wanted to point out why this was done!

…ifferent serializer for the response versus the request.
 into feature/node_settings_apiV2

- Catch NodeStateErrors when running set_editing and fix but in to_representation.
@pattisdr pattisdr force-pushed the feature/node_settings_apiV2 branch from 6b9cb9c to ab17e69 Compare July 19, 2018 18:10
redirect_link_url = ser.URLField(write_only=True, required=False)
redirect_link_label = ser.CharField(max_length=50, write_only=True, required=False)

def to_representation(self, instance):
Copy link
Contributor Author

@pattisdr pattisdr Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overrode to_representation so a PATCH request could use the NodeSettingsUpdateSerializer, but the response would use the NodeSettingsSerializer. This was one of the suggestions on this issue: Specifying different serializers for input and output

Other ideas I considered before going this route:

  • Different field names for write versus read
  • https://github.com/vintasoftware/drf-rw-serializers/ - separated serializers for read/write operations
  • Overriding put method to instantiate different serializers depending on write versus read, and then return response(serialized_data)

"""
context = self.context
context['wiki_addon'] = instance.get_addon('wiki')
context['forward_addon'] = instance.get_addon('forward')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing in the addons in the context, because the NodeSettingsSerializer currently expects it. This was placed on the context to speed up the request so we weren't repeatedly fetching the same addons.

@sloria sloria merged commit 54b2758 into CenterForOpenScience:develop Jul 27, 2018
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.

None yet

3 participants