Skip to content

[BB-7296] Extend settings handler to be accessible via api#32127

Closed
pkulkark wants to merge 1 commit intoopenedx:masterfrom
open-craft:pooja/bb7296-expose-settings-handler-to-api
Closed

[BB-7296] Extend settings handler to be accessible via api#32127
pkulkark wants to merge 1 commit intoopenedx:masterfrom
open-craft:pooja/bb7296-expose-settings-handler-to-api

Conversation

@pkulkark
Copy link
Contributor

@pkulkark pkulkark commented Apr 25, 2023

Description

This PR adds a new endpoint details_settings that allows fetching and updating the course details settings, which previously could only be updated with a session-based request. With these changes, it can be updated with a token-based request. The changes are similar to what already exists for advanced_settings.

Supporting information

OpenCraft Internal Jira ticket: BB-7296

Testing instructions

  1. Checkout this branch and provision devstack.
  2. Verify that the new api details show up in http://localhost:18010/api-docs/.
  3. Using a token-based authentication, verify that the methods GET and PATCH return the expected results.

@openedx-webhooks
Copy link

openedx-webhooks commented Apr 25, 2023

Thanks for the pull request, @pkulkark!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

@pkulkark pkulkark changed the title [WIP][BB-7296] Extend settings handler to be accessible via api [BB-7296] Extend settings handler to be accessible via api May 8, 2023
@pkulkark pkulkark marked this pull request as ready for review May 8, 2023 17:50
@pkulkark pkulkark force-pushed the pooja/bb7296-expose-settings-handler-to-api branch 6 times, most recently from 83a189f to e1a440d Compare May 11, 2023 18:49
Copy link
Contributor

@0x29a 0x29a left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this.
  • I read through the changes.

@mphilbrick211
Copy link

Hi @pkulkark! Would you mind resolving the branch conflicts here? I will look into getting this reviewed. Thanks!

@mphilbrick211 mphilbrick211 reopened this Jun 21, 2023
@pkulkark pkulkark force-pushed the pooja/bb7296-expose-settings-handler-to-api branch from e1a440d to e8bfec0 Compare June 22, 2023 15:57
@pkulkark pkulkark force-pushed the pooja/bb7296-expose-settings-handler-to-api branch from e8bfec0 to 8bb915c Compare June 22, 2023 16:15
@pkulkark
Copy link
Contributor Author

@mphilbrick211 Done. Please go ahead with the review.

@mphilbrick211
Copy link

Hi @feanil! Ed thought you might be able to review/merge this one. Let me know if not. Thanks!

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

At the very least we need to explain what the content of the patch body would look like and how a user of the API can know what valid values are.

@verify_course_exists()
def patch(self, request: Request, course_id: str):
"""
Update a course's details settings.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't tell me anything about what settings can be in the patch. What settings can actually be updated by this API endpoint? Is that already documented somewhere and you can just point to that from here? If not, can you provide a list of the values that can be in the patch body? Also, is it possible to write a serializer for the data being output and input for these views? That would solve both the documentation issue and could be used to perform some basic validation on the input.

Copy link
Contributor Author

@pkulkark pkulkark Jul 4, 2023

Choose a reason for hiding this comment

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

@feanil This is meant to allow API access to the existing settings_handler which currently only allow session-based requests. It doesn't change any of its functionality. We added serializers for it in a separate django app (c.f. ref) which consumes this api along with the existing advanced_settings api. But you're right, I think we could still add one here like the CourseAdvancedSettingsSerializer. I'll add it and update here.

@pkulkark
Copy link
Contributor Author

pkulkark commented Jul 4, 2023

@feanil Turns out this feature was merged to master recently via another PR - #32397. So this is no longer needed. Closing it.

@pkulkark pkulkark closed this Jul 4, 2023
@openedx-webhooks
Copy link

@pkulkark Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@Agrendalath Agrendalath deleted the pooja/bb7296-expose-settings-handler-to-api branch July 21, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core committer open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants