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

fix: set JSONRenderer as the DEFAULT_RENDERER_CLASSES #35

Closed

Conversation

CodeWithEmad
Copy link
Member

in production environments, it's not the best practice to have a browsable API and normally only a json should be returned.

Close #34

@CodeWithEmad
Copy link
Member Author

IDK if using ENABLE_HTTPS as the condition for local and prod envs is the best decision or not.
also, something went south with the add-to-project job and github-token

Run actions/[email protected]
Error: Input required and not supplied: github-token

@jfavellar90

@jfavellar90
Copy link
Collaborator

@CodeWithEmad thanks for the PR. From my experience, ENABLE_HTTPS is not the best variable to determine whether it's a production environment. I wonder if this is a fix that should be enforced directly in the edx-notes codebase to follow the best practices from there and prevent us from adding these adjustments to be security-compliant. Is it OK for you to handle this extra setting as a plugin till getting a long-term solution?

Regarding the action that is failing, I'm not sure what's going on, I suspect the Token used in this workflow was removed. @regisb could you confirm? There are failures in a similar workflow in the Tutor repo

@CodeWithEmad
Copy link
Member Author

Thanks for the response, @jfavellar90

From my experience, ENABLE_HTTPS is not the best variable to determine whether it's a production environment

I appreciate any suggestions here. I wish there were an ENV variable inside tutor. :)

I wonder if this is a fix that should be enforced directly in the edx-notes codebase

At first, I went there to change the config, but they store essential configs inside base.py and import that for different configs like dev, devstack, test, etc. I can add this change to those places as well, but since we create/use a tutor.py config and use that as the DJANGO_SETTINGS_MODULE, I don't see why we should not change it here.

@CodeWithEmad CodeWithEmad requested a review from regisb March 11, 2024 09:10
in production environments, it's not the best practice to have a browsable API and normally only a json should be returned.

Close overhangio#34
@CodeWithEmad CodeWithEmad force-pushed the fix/browsable-renderer branch from 93adf14 to c52eb18 Compare March 11, 2024 09:12
@regisb
Copy link
Collaborator

regisb commented Mar 21, 2024

What is the issue that we are trying to fix here? Is it that the API browser is badly displayed, or is that that the API browser should not be displayed at all? What behaviour would we like to have, both in production and development?

Do we want to have a JSON-rendered API in production? If yes, then checking for HTTPS is not the right way to detect production settings. If we should have different settings in production and development, then we should either have different setting files, or define ENV variables in both environments.

But if we do want to have a browseable API in production, then we should configure uwsgi/gunicorn to serve static assets.

@CodeWithEmad
Copy link
Member Author

CodeWithEmad commented Mar 22, 2024

Well, I think having a Browsable API in production is not necessarily a bad thing, but this is a common pattern, where only JSON Renderer is used on the production environment due to some security and performance issues.
Having or not having it is your choice. Either I can use DEBUG = False in the settings and keep the change in tutor.py or I can fix the UWSGI configs to serve static assets in production and keep the Browsable API.

@regisb
Copy link
Collaborator

regisb commented Mar 26, 2024

Having or not having it is your choice. Either I can use DEBUG = False in the settings and keep the change in tutor.py or I can fix the UWSGI configs to serve static assets in production and keep the Browsable API.

I'm happy with either. What do you think @jfavellar90?

@kdmccormick
Copy link
Contributor

from Tutor users' group:
image

@CodeWithEmad
Copy link
Member Author

As we discussed in Tutor users' group, it's a better approach to add this change to edx-notes project, like what we've in edx-platform.

@CodeWithEmad CodeWithEmad deleted the fix/browsable-renderer branch April 22, 2024 15:29
kdmccormick pushed a commit to openedx/edx-notes-api that referenced this pull request May 6, 2024
This will introduce a similar approach as edx-platform to set JSONRenderer
as the DEFAULT_RENDERER_CLASSES. with this, we don't have Browsable API
Anymore. also, some RST cleanup in README.

See here for more context: overhangio/tutor-notes#35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Won't fix
Development

Successfully merging this pull request may close these issues.

BrowsableAPIRenderer in production
4 participants