-
Notifications
You must be signed in to change notification settings - Fork 1
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
[ACADEMIC-16210] Moved API logic to api module and added check #43
Conversation
530a338
to
bc9a3b7
Compare
b1dd198
to
25bcaaa
Compare
25bcaaa
to
4ddf700
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some random nitpicking comments about names and return values that honestly I don't care whether you take them or not.
The one thing that makes me hit request changes is that we do need the exposed API function and view and URL exposing the waffle flag to know if configuration is even allowed for the course.
ai_aside/api/api.py
Outdated
@@ -0,0 +1,138 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a trick you can do here is to put the api calls into init.py inside the api directory, then you won't have api.api going on.
if you do that it would probably make sense to put just the public items in there and put the _get style internal methods in like api/internal.py
honestly I'm not a huge fan of putting a bunch of stuff in the init.py but it is a very common practice
alternately, what if we called the api directory the config_api directory, that sets the context for the views and the api.py file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went for the config_api
directory but keeping the api.py
. Tried a few things and it went weird. I did move the _get_*
methods to a different internal.py
module.
ai_aside/api/api.py
Outdated
|
||
|
||
def get_course_settings(course_key): | ||
"Gets the settings of a course" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the get and set are using key/value dicts, you should describe the expected dicts in the docstring
ai_aside/api/api.py
Outdated
if not isinstance(enabled, bool): | ||
raise TypeError | ||
|
||
update = {'enabled': enabled} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this just settings?
ai_aside/api/api.py
Outdated
defaults=update, | ||
) | ||
|
||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this return value doesn't really mean anything so don't bother to return anything
ai_aside/api/api.py
Outdated
if not isinstance(enabled, bool): | ||
raise TypeError | ||
|
||
update = {'enabled': enabled} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as in course
ai_aside/api/api.py
Outdated
defaults=update, | ||
) | ||
|
||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meaningless return
a common thing you could do instead is to return the same thing the GET would - so in this case the settings dict again - but that's still pretty meaningless. In the future case where you are updating only some settings it would be more useful to return the complete settings dict incorporating changes afterwards, but we only have the one
ai_aside/api/api.py
Outdated
if course is not None: | ||
return course.enabled | ||
|
||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need one more function in the API (and views) that just exposes summaries_configuration_enabled(course_key), since neither the course level or unit level settings should appear if that is not on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this to the PR to unblock David while Marcos is out sick so this should be all set
ai_aside/block.py
Outdated
return summary_enabled(course_key) | ||
|
||
if not ff_summary_enabled(course_key): | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this clause should only trigger if the new flag is not on - we'll be retiring this old flag and replacing this with configuration, so it will start to be off everywhere. You could move this check into is_summary_enabled, something like:
if summary config waffle is enabled for this course:
check the config
otherwise if the old flag is enabled for this course:
return true
return false
8f4a209
to
dc198b6
Compare
2d40b17
to
4c188ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 after cleaning out the stray requirements.txt file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I missed one needed logic change
ai_aside/config_api/api.py
Outdated
|
||
# If the feature flag is disabled, do not restrict | ||
if not summaries_configuration_enabled(course_key): | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be false
this function is only for use when using the config settings
if you're not allowed to use the config settings, you shouldn't be here and shouldn't be allowed to do anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It was returning True
just to keep the current logic for the blocks to be enabled by default. Now that the should_apply_to_block
is using it only when the waffle flag is enabled, any other uses of the method make sense to return False
if the flag is disabled. Updated.
dfcf614
to
873129b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment to fix before merge but then good to go
ai_aside/config_api/api.py
Outdated
It considers both the state of a unit's override and a course defaults. | ||
""" | ||
|
||
# If the feature flag is disabled, do not restrict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code is updated but he comment isn't
- Added endpoint exposing is this course allowed to configure summaries - Added tests for new endpoint - Added more tests while introcucing the DDT library Co-authored-by: Andy Shultz <ashultz@edx.org>
873129b
to
3d113d2
Compare
[https://jira.2u.com/browse/ACADEMIC-16209](https://jira.2u.com/browse/ACADEMIC-16209) 1. Add unit xpert unit summaries settings behind flag `summaryhook_summaries_configuration` added [here](https://github.com/edx/ai-aside/pull/45/files) 2. Only show the checkbox when the value is a `boolean` otherwise the feature is considerer `disabled` by the flag. 3. Update block handler to update this value via `api` exposed [here](edx/ai-aside#43) 4. Create `AiAsideSummary` configuration class to provide access to the `ai_aside.api` endpoints.
[https://jira.2u.com/browse/ACADEMIC-16209](https://jira.2u.com/browse/ACADEMIC-16209) 1. Add unit xpert unit summaries settings behind flag `summaryhook_summaries_configuration` added [here](https://github.com/edx/ai-aside/pull/45/files) 2. Only show the checkbox when the value is a `boolean` otherwise the feature is considerer `disabled` by the flag. 3. Update block handler to update this value via `api` exposed [here](edx/ai-aside#43) 4. Create `AiAsideSummary` configuration class to provide access to the `ai_aside.api` endpoints.
[https://jira.2u.com/browse/ACADEMIC-16209](https://jira.2u.com/browse/ACADEMIC-16209) 1. Add unit xpert unit summaries settings behind flag `summaryhook_summaries_configuration` added [here](https://github.com/edx/ai-aside/pull/45/files) 2. Only show the checkbox when the value is a `boolean` otherwise the feature is considerer `disabled` by the flag. 3. Update block handler to update this value via `api` exposed [here](edx/ai-aside#43) 4. Create `AiAsideSummary` configuration class to provide access to the `ai_aside.api` endpoints.
[https://jira.2u.com/browse/ACADEMIC-16209](https://jira.2u.com/browse/ACADEMIC-16209) 1. Add unit xpert unit summaries settings behind flag `summaryhook_summaries_configuration` added [here](https://github.com/edx/ai-aside/pull/45/files) 2. Only show the checkbox when the value is a `boolean` otherwise the feature is considerer `disabled` by the flag. 3. Update block handler to update this value via `api` exposed [here](edx/ai-aside#43) 4. Create `AiAsideSummary` configuration class to provide access to the `ai_aside.api` endpoints.
[https://jira.2u.com/browse/ACADEMIC-16209](https://jira.2u.com/browse/ACADEMIC-16209) 1. Add unit xpert unit summaries settings behind flag `summaryhook_summaries_configuration` added [here](https://github.com/edx/ai-aside/pull/45/files) 2. Only show the checkbox when the value is a `boolean` otherwise the feature is considerer `disabled` by the flag. 3. Update block handler to update this value via `api` exposed [here](edx/ai-aside#43) 4. Create `AiAsideSummary` configuration class to provide access to the `ai_aside.api` endpoints.
[https://jira.2u.com/browse/ACADEMIC-16209](https://jira.2u.com/browse/ACADEMIC-16209) 1. Add unit xpert unit summaries settings behind flag `summaryhook_summaries_configuration` added [here](https://github.com/edx/ai-aside/pull/45/files) 2. Only show the checkbox when the value is a `boolean` otherwise the feature is considerer `disabled` by the flag. 3. Update block handler to update this value via `api` exposed [here](edx/ai-aside#43) 4. Create `AiAsideSummary` configuration class to provide access to the `ai_aside.api` endpoints.
* feat: [ACADEMIC-16209] Unit summary settings [https://jira.2u.com/browse/ACADEMIC-16209](https://jira.2u.com/browse/ACADEMIC-16209) 1. Add unit xpert unit summaries settings behind flag `summaryhook_summaries_configuration` added [here](https://github.com/edx/ai-aside/pull/45/files) 2. Only show the checkbox when the value is a `boolean` otherwise the feature is considerer `disabled` by the flag. 3. Update block handler to update this value via `api` exposed [here](edx/ai-aside#43) 4. Create `AiAsideSummary` configuration class to provide access to the `ai_aside.api` endpoints.
* feat: [ACADEMIC-16209] Unit summary settings [https://jira.2u.com/browse/ACADEMIC-16209](https://jira.2u.com/browse/ACADEMIC-16209) 1. Add unit xpert unit summaries settings behind flag `summaryhook_summaries_configuration` added [here](https://github.com/edx/ai-aside/pull/45/files) 2. Only show the checkbox when the value is a `boolean` otherwise the feature is considerer `disabled` by the flag. 3. Update block handler to update this value via `api` exposed [here](edx/ai-aside#43) 4. Create `AiAsideSummary` configuration class to provide access to the `ai_aside.api` endpoints.
summaryhook.summaryhook_summaries_configuration
waffle flag.Merge checklist:
Check off if complete or not applicable: