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

Add KOLIBRI_ENABLE_CUSTOM_CHANNEL_NAV - default True to options.py #8025

Merged

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented Apr 23, 2021

Summary

Adds an option that will be picked up from the options.ini file or by env var called DISABLE_CUSTOM_CHANNEL_NAV.

It is not then used anywhere as (I believe) it is intended for future debugging and/or dev use during development of the new feature.

References

Fixes #7830

Reviewer guidance

See that I did what is said in the well written and clear documentation that I ought to have read

@nucleogenesis nucleogenesis added the TODO: needs review Waiting for review label Apr 23, 2021
@nucleogenesis nucleogenesis added this to the 0.15.0 milestone Apr 23, 2021
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Let's scope this to the learn plugin.

Would also be helpful to expose this in the plugin data in Learn for ease of use: https://github.com/learningequality/kolibri/blob/release-v0.14.x/kolibri/plugins/learn/kolibri_plugin.py#L51

kolibri/utils/options.py Outdated Show resolved Hide resolved
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

A couple more tweaks - sorry!

"LEARN": {
"DISABLE_CUSTOM_CHANNEL_NAV": {
"type": "boolean",
"default": False,
Copy link
Member

Choose a reason for hiding this comment

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

The issue asks for this to be disabled by default, so if the flag is going to be DISABLE then this should default to True!

Copy link
Member Author

Choose a reason for hiding this comment

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

Made it into an ENABLE name instead

@@ -0,0 +1,9 @@
option_spec = {
"LEARN": {
Copy link
Member

Choose a reason for hiding this comment

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

If you're editing the default, can also make this Title Case, which seems to be our unwritten convention for options section names.

"DISABLE_CUSTOM_CHANNEL_NAV": {
"type": "boolean",
"default": False,
"envvars": ("DISABLE_CUSTOM_CHANNEL_NAV",),
Copy link
Member

Choose a reason for hiding this comment

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

We've also tended to prefix these with KOLIBRI_ - there's an open issue to just generate these names automatically to avoid this hullabaloo: #5754

If we can just prefix KOLIBRI_ here for now, would be good.

@rtibbles rtibbles merged commit 9af85e8 into learningequality:develop Apr 29, 2021
@jonboiser jonboiser changed the title Add DISABLE_CUSTOM_CHANNEL_NAV - default false to options.py Add KOLIBRI_ENABLE_CUSTOM_CHANNEL_NAV - default True to options.py Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
2 participants