feat: Add REST APIs for course advanced settings and course tabs [BD-38] [TNL-8625] [BB-4132]

[xitij2000]

This PR adds new APIs that allow MFEs to access and update course advanced settings, and to get the course tabs and show/hide/reorder them.

Sandbox URL:

• LMS: https://pr27608.sandbox.opencraft.hosting/
• Studio: https://studio.pr27608.sandbox.opencraft.hosting/

Merge deadline: "None"

Testing instructions:

1. Run the added unit tests.
2. You can see the data about all available course advanced settings using the same format used by studio by visiting: http://localhost:18010/api/contentstore/v0/advanced_settings/course-v1:edX+DemoX+Demo_Course
3. You can make a post request to the above using data formatted as:

{
  "disable_progress_graph": {
     "value": false  
   }
}

4. Check that the list of tabs and their settings in the course using the tabs api: http://localhost:18010/api/contentstore/v0/tabs/course-v1:edX+DemoX+Demo_Course
5. Try hiding a tab by posting ``{"is_hidden": true"}` to http://localhost:18010/api/contentstore/v0/tabs/course-v1:edX+DemoX+Demo_Course/settings?tab_id=progress
6. The tab should be hidden from the UI
7. Test rearranging tabs by posting a new tab order to http://localhost:18010/api/contentstore/v0/tabs/course-v1:edX+DemoX+Demo_Course/reorder eg:

[
    {
        "tab_id": "info"
    },
    {
        "tab_id": "courseware"
    },
    {
        "tab_id": "wiki"
    },
    {
        "tab_id": "discussion"
    },
    {
        "tab_id": "progress"
    }
]

Reviewers

• (OpenCraft internal reviewer's GitHub username goes here)
• edX reviewer[s] TBD

TNL-8625

[openedx-webhooks]

Thanks for the pull request, @xitij2000! I've created BLENDED-841 to keep track of it in Jira. More details are on the BD-38 project page.

When this pull request is ready, tag your edX technical lead.

[stvstnfrd]

just passing by...

[stvstnfrd]

If we're creating a new API endpoint, shouldn't that use a higher number instead of a lower number?
If the first/pre-existing value was 1, I'd assume the list was 1-indexed and the next number should be 2, not 0.

[xitij2000]

I've called it v0 because in this case, the API is just exposing an existing endpoint without too much thought put into the structure of the response etc. It's so we can get started with using this from MFEs quicker. I'd expect a v1 of the API as the next iteration that optimises how this works in a way that works better for MFEs.

For instance, I'd expect this API to be split into multiple other APIs, one for each app that can be used to configure it, instead of one single API for all misc operations.

On the other hand, if we can maintain this API as-is, I don't see any reason to not call it v1, and just do the next iteration as v2.

[stvstnfrd]

I dig all the added type hints!

[kewne]

LGTM, only minor suggestions.

[asadazam93]

@xitij2000 Can you please add a more detailed description of this PR? There are a lot of changes and it seems to me it is exposing an api for advanced settings, but I have no context like, where will this api be used? what fields do we need in the API? etc

[awaisdar001]

@xitij2000 I had a bunch of questions for this PR, sorry. I am missing some context and information about this PR and linked ticket.

[awaisdar001]

Hi @xitij2000 added a few questions and concerns, no major blockers.

[xitij2000]

jenkins run python

[xitij2000]

@awaisdar001 Is this good to merge?

[awaisdar001]

@awaisdar001 Is this good to merge?

I am taking a look what have left here.

[xitij2000]

@xitij2000 Is it possible to move the course tabs related apis to a separate PR? Since we do not need that right now.

It is definitely possible, but I think it's a fine balance whether it's more effort to split the PR or review it as is.

If it's a matter of review capacity, I think @Agrendalath can help with the review as a core committer.

[awaisdar001]

I think we need to address the following concerns / questions before merging this.

1. The size (in terms of data) of the v0/advanced_settings is a concern. Its 48kb call comparing with 1.7kb call for pages & resources. Do you think we can cap this call with list of fields/advance settings we need just how e.g. facebook cap the calls to the fields which are most-relevant/requested by end user? Do you think we need all these settings? Can this call be capped? This will help in decreasing the api call size as well as it'd be easier process the data.

we can keep the "Return all fields" feature as well if user doesn't specify any fields, but i think its a good-to-have feature given the data-size.

2. I have a question regarding the params passed for toggling the settings. there are 2 types of params used for the POST request. Query & Post Request params. Can you explain why its make more sense to pass tab_id=progress as query param and toggle settings is_hidden as request.post param Vs passing both as request.post params? v0/tabs/

3. As python maintains ordering in the list, don't you think it'd be easier to pass ordering as a list of tab_ids? e.g. [tab_id_1, tab_id_2] rather the current list-of-objects?

[Agrendalath]

If it's a matter of review capacity, I think @Agrendalath can help with the review as a core committer.

@xitij2000, sure, feel free to ping me if we need an additional review.

[xitij2000]

@awaisdar001
I think the issue here is that we had a very specific scope for this PR, which is explained in the ADR I created for this work: https://github.com/edx/edx-platform/pull/27129

The idea was to speed up work on pages and resources, we can take the existing studio APIs for course advanced settings and tabs and expose them via new API endpoints that can be use by MFE.

So the aim was for this to be a stopgap v0 API, since it's a small bit of work, and unblocks further progress here. Future iterations of the APIs can be created with a new design. So the size of the API etc are the same as the existing APIs, since it was never the aim to change them.

I think the best time to advocate for such a change in approach would have been during the ADR phase, since once it was approved we started work on that basis, and at that stage it would have been easier to build the APIs with these changes in mind.

Do you think we need all these settings? Can this call be capped?

No we don't need all these settings, and this call can be capped, will update this to add that.

I have a question regarding the params passed for toggling the settings. there are 2 types of params used for the POST request. > Query & Post Request params. Can you explain why its make more sense to pass tab_id=progress as query param and toggle > settings is_hidden as request.post param Vs passing both as request.post params?

I've split them this way since the tab id or tab location is specifying the resource, while the actual data is part of the post body.

As python maintains ordering in the list, don't you think it'd be easier to pass ordering as a list of tab_ids? e.g. [tab_id_1, tab_id_2] rather the current list-of-objects?

Sure, however we can have a mix of tab ids and tab locations, which are usage keys, so we need to know if it's a tab_id or a tab_location, so we specify it in an object structure to be clear.

[asadazam93]

Looks good

[asadazam93]

@xitij2000 Is this ready to be merged?

[xitij2000]

@xitij2000 Is this ready to be merged?

I am currently working on updating the PR to allow partial queries. Will push that change soon.

[xitij2000]

@xitij2000 Is this ready to be merged?

I've updated the PR to add support for filtering fields.

[awaisdar001]

thanks for the improvement @xitij2000 - i'll review the changes on my devstack today.

[awaisdar001]

Looks good to me 👍

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[xitij2000]

@awaisdar001 This is good to merge from my side.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.