Skip to content

[ACADEMIC-16210] Add endpoints for enabling courses/modules - #38

Merged
rijuma merged 1 commit into
mainfrom
rijuma/16210-gate-summary-aside-config
Jul 21, 2023
Merged

[ACADEMIC-16210] Add endpoints for enabling courses/modules#38
rijuma merged 1 commit into
mainfrom
rijuma/16210-gate-summary-aside-config

Conversation

@rijuma

@rijuma rijuma commented Jul 18, 2023

Copy link
Copy Markdown
Contributor

Added endpoints for setting/getting the enabled state on the Courses and Course Units.

Setters:

  • POST: ai_aside/v1/:course_id - (payload: { enabled: True/False })
  • POST: ai_aside/v1/:course_id/:unit_id - (payload: { enabled: True/False })

Getters:

  • GET: ai_aside/v1/:course_id - (response: { success: True/False, enabled: True/False })
  • GET: ai_aside/v1/:course_id/:unit_id - (response: { success: True/False, enabled: True/False })

Delete:

  • DELETE: ai_aside/v1/:course_id - (response: { success: True/False })
  • DELETE: ai_aside/v1/:course_id/:unit_id - (response: { success: True/False })

Merge checklist:
Check off if complete or not applicable:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

@rijuma rijuma added the WIP Work in Progress label Jul 18, 2023
@rijuma
rijuma force-pushed the rijuma/16210-gate-summary-aside-config branch 4 times, most recently from 34ddd62 to 72013b7 Compare July 18, 2023 15:10
Comment thread ai_aside/api/views.py Outdated
@rijuma
rijuma force-pushed the rijuma/16210-gate-summary-aside-config branch 5 times, most recently from 1deb180 to 5e5cf1a Compare July 19, 2023 19:23
@rijuma rijuma removed the WIP Work in Progress label Jul 19, 2023
@rijuma
rijuma marked this pull request as ready for review July 19, 2023 19:24
Comment thread ai_aside/.DS_Store Outdated
Comment thread ai_aside/__init__.py Outdated
Comment thread test_settings.py
@rijuma
rijuma force-pushed the rijuma/16210-gate-summary-aside-config branch 2 times, most recently from 750d061 to 41ef8a6 Compare July 19, 2023 19:40
@aethant

aethant commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

@rijuma @ashultz0 to play devil's advocate here, and based on Andy's thread in Slack earlier, would it be beneficial to use string flags rather than booleans to account for 'gray areas' of summary enabling?

@ashultz0 ashultz0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the structure is generally fine but there are a bunch of non-obvious things about how django queries work that we should change

haven't yet had a look at the tests, I wanted to get this core stuff marked first

Comment thread ai_aside/api/urls.py Outdated
Comment thread ai_aside/api/views.py Outdated
Comment thread ai_aside/api/views.py Outdated
Comment thread ai_aside/api/views.py Outdated

AIAsideCourseEnabled.objects.update_or_create(
course_key=course_key,
enabled=enabled,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think to make this work right we have to update the model to tell it that course_key is a unique key, that way it can know that there already is one and it should update. Without that it might also take enabled into account so you could have two, or it might always create.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the DELETE http word for removing the override. Also updated the models to make sure we use unique rows for the settings.

Comment thread ai_aside/api/views.py Outdated
@rijuma

rijuma commented Jul 19, 2023

Copy link
Copy Markdown
Contributor Author

@rijuma @ashultz0 to play devil's advocate here, and based on Andy's thread in Slack earlier, would it be beneficial to use string flags rather than booleans to account for 'gray areas' of summary enabling?

Leaving a paper trail here, the issue is that not having an override on a module indicates that the enabled state is determined by the course, so for the modules we would have three states:

  • enabled
  • disabled
  • inherited from the course

So the module record is considered as an override for the course.
I'll add the option to delete the record for removing the override.

Thanks @aethant for bringing this up.

@ashultz0

Copy link
Copy Markdown
Contributor

@rijuma @ashultz0 to play devil's advocate here, and based on Andy's thread in Slack earlier, would it be beneficial to use string flags rather than booleans to account for 'gray areas' of summary enabling?

Interesting... though since we are installing into a lack of settings, we're forced to handle the case without settings for courses and units no matter what field type we have here.

I was trying to feel out whether we're going to have a followup project that is about having different types of enabled summary-like thing but there is nothing on the radar so that won't be soon. That would be something like OFF/SUMMARIES/REVIEW_QUESTIONS/JOKES keying to different AI types. It felt like borrowing work to put it in now although we could just put it in at the DB layer and then expose just the boolean via the API, preventing a possible future migration.

@aethant

aethant commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

@rijuma @ashultz0 to play devil's advocate here, and based on Andy's thread in Slack earlier, would it be beneficial to use string flags rather than booleans to account for 'gray areas' of summary enabling?

Interesting... though since we are installing into a lack of settings, we're forced to handle the case without settings for courses and units no matter what field type we have here.

I was trying to feel out whether we're going to have a followup project that is about having different types of enabled summary-like thing but there is nothing on the radar so that won't be soon. That would be something like OFF/SUMMARIES/REVIEW_QUESTIONS/JOKES keying to different AI types. It felt like borrowing work to put it in now although we could just put it in at the DB layer and then expose just the boolean via the API, preventing a possible future migration.

Were it me - I would probably do something like <TYPE>_<STATE1>, <TYPE>_<STATE2>, and so on. Different potential tools may have different toggle states, even for the same content. A syllabus is only really valid, for example, at a higher level than an individual unit or lesson. In this case, you might have SUMMARY_OFF SUMMARY_ON SUMMARY_INHERIT or something. Then, too, you can always add new types down the road as new issues arise.

@ashultz0

Copy link
Copy Markdown
Contributor

currently the only UX that removes the override is when the course level setting is changed it wipes all existing unit configs, so that could be embedded in the course level API call... but having it separate would be more reusable later. There is no UX for wiping a single unit at a time right now.

@ashultz0

Copy link
Copy Markdown
Contributor

We can use multiple fields and migrate in a config type field later defaulted to summary to update all existing enabled/disabled into the right bucket when we have more buckets. We don't know much about future use cases so I'm reluctant to suggest more complexity.

@rijuma
rijuma force-pushed the rijuma/16210-gate-summary-aside-config branch 3 times, most recently from 8402e91 to 2be0cb5 Compare July 20, 2023 17:51
operations = [
migrations.AlterModelOptions(
name='aiasidecourseenabled',
options={},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was trying to figure out why it was doing this and it must be removing the default sort 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, probably

@rijuma
rijuma force-pushed the rijuma/16210-gate-summary-aside-config branch from 2be0cb5 to 4960691 Compare July 20, 2023 18:25
@rijuma
rijuma requested a review from ashultz0 July 20, 2023 18:45
@ashultz0

Copy link
Copy Markdown
Contributor

please capture the description you have at the top of this PR in the repo somewhere - honestly just fleshing out the docstring at the top of the views file is probably as fancy as needed. Since you wrote it, let's not lose it :)

Comment thread tests/test_api.py
})
response = self.client.delete(api_url)

self.assertEqual(response.status_code, 404)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

by adding the unique constraints we now have another useful case to test: that when you do a post with the same course_id or course_id/unit_id pair and a different enabled it does not create a new record

@rijuma rijuma Jul 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is already there in the unit tests. For the "disabled" cases I added a record as true and then run the endpoint to set it as false, checking that only one record remains at the end.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah I see, I read too fast and missed that there was a direct create and a post, perfectly valid way to do it 👍

@ashultz0 ashultz0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's ship it! Please capture your good doc but no need for more review

@rijuma
rijuma force-pushed the rijuma/16210-gate-summary-aside-config branch from 4960691 to 30907fd Compare July 21, 2023 14:24
@rijuma
rijuma merged commit 4cb4eee into main Jul 21, 2023
@rijuma
rijuma deleted the rijuma/16210-gate-summary-aside-config branch July 21, 2023 14:31
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.

3 participants