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

Department REST API #1877

Merged
merged 23 commits into from
Sep 12, 2023
Merged

Department REST API #1877

merged 23 commits into from
Sep 12, 2023

Conversation

collinpreston
Copy link
Contributor

What are the relevant tickets?

#1876

Description (What does it do?)

Adds a REST API endpoint for retrieving departments in MITx Online. The endpoint is http://mitxonline.odl.local:8013/api/departments. The data returned from the endpoint is:

[
    {
        "name": "Science",
        "courses": 2,
        "programs": 0
    },
    {
        "name": "Math",
        "courses": 1,
        "programs": 0
    },
    {
        "name": "test",
        "courses": 1,
        "programs": 1
    }
]

Calls to this endpoint are added from the catalog page in order to display all departments for courses or programs upon initial load of the page.

Departments are only displayed based on the following:

  1. If the current tab is "courses", the department must have 1 or more courses associated with it.
  2. If the current tab is "programs, the department must have 1 or more programs associated with it.

How can this be tested?

  1. Create 13 courses and course runs which are expected to be displayed in the catalog.
  2. Add a unique department to the 13th course.
  3. Load the catalog page. Verify that the unique department associated with the 13th course is shown even if the 13th course has not yet been displayed on the catalog page.

Additional Context

There is the possibility that a department is associated with 1 or more courses or programs that will not be visible on the catalog page due to the catalogs business logic for filtering courses and programs. The department will still be shown.

@collinpreston collinpreston linked an issue Sep 11, 2023 that may be closed by this pull request
@collinpreston
Copy link
Contributor Author

collinpreston commented Sep 11, 2023

This PR is branched from #1872 and should be blocked until that PR is merged.

@Ferdi
Copy link

Ferdi commented Sep 11, 2023

Why just the nr of course and not the list of courses ? Is there any api/departments/[departmentid]/courses endpoint ?

@collinpreston collinpreston marked this pull request as ready for review September 11, 2023 19:23
@collinpreston
Copy link
Contributor Author

collinpreston commented Sep 11, 2023

Why just the nr of course and not the list of courses ? Is there any api/departments/[departmentid]/courses endpoint ?

@Ferdi I was thinking about including the list of course ID's since you could then use those to make a second API call to retrieve the courses themselves. I didn't want this endpoint to become a substitute for the courses endpoint by including too much course information.

@jkachel jkachel self-requested a review September 12, 2023 13:17
@jkachel jkachel self-assigned this Sep 12, 2023
Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

Seems to work OK but there's some weirdness with the DRF frontend noted below.

Comment on lines 519 to 521
queryset = Department.objects.annotate(
courses=Count("course"), programs=Count("program")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be moved to a get_queryset? It breaks rendering with the DRF frontend otherwise - if you just try to hit the API from the browser directly, you get a 500 error because there's no queryset property on the class. (It's fine from within the app or if you set ?format=json, so this just affects the frontend that DRF adds on.)

@Ferdi
Copy link

Ferdi commented Sep 12, 2023

@collinpreston thanks, is there any endpoint do you get a list of courses in a given department (api/departments/[departmentid]/courses ) ?

@collinpreston
Copy link
Contributor Author

@collinpreston thanks, is there any endpoint do you get a list of courses in a given department (api/departments/[departmentid]/courses ) ?

@Ferdi , there is not.

Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@collinpreston collinpreston merged commit f49e9c6 into main Sep 12, 2023
@collinpreston collinpreston deleted the department-rest-api branch September 12, 2023 15:33
collinpreston added a commit that referenced this pull request Sep 12, 2023
collinpreston added a commit that referenced this pull request Sep 12, 2023
* Revert "Department REST API (#1877)"

This reverts commit f49e9c6.

* Revert "1869: Course program api performance improvements (#1872)"

This reverts commit f16b217.
@odlbot odlbot mentioned this pull request Sep 12, 2023
4 tasks
collinpreston added a commit that referenced this pull request Sep 12, 2023
collinpreston added a commit that referenced this pull request Sep 12, 2023
collinpreston added a commit that referenced this pull request Sep 12, 2023
* Revert "Revert "Department REST API (#1877)" (#1882)"

This reverts commit c87bdc1.

* Revert "Department REST API (#1877)"

This reverts commit f49e9c6.

* format

* satisfy flow

* flow and me
@odlbot odlbot mentioned this pull request Sep 12, 2023
5 tasks
collinpreston added a commit that referenced this pull request Sep 14, 2023
collinpreston added a commit that referenced this pull request Sep 14, 2023
* Revert "Revert "Department REST API (#1877)" (#1882)"

This reverts commit c87bdc1.

* Use site name for title on index page template

* Revert "Revert "Revert "Department REST API (#1877)" (#1882)""

This reverts commit 38c6578.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not all departments are displaying when the catalog page loads
3 participants