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

feat: Remove JWT_AUTH_REFRESH_COOKIE Setting depr190 #32647

Closed
wants to merge 31 commits into from

Conversation

Yagnesh1998
Copy link
Contributor

Description:
The setting JWT_AUTH_REFRESH_COOKIE is meaningless and unused and should be cleaned up to avoid confusion.
In the very early days of introducing MFEs, we thought we were going to need this cookie in addition to the JWT cookie. However, it turned out we didn't need it, but the setting stuck around the contagion of it (being in cookiecutter and other template libraries) has resulted in it uselessly being copied to many repos.

Supporting information:
as per the original ticket openedx/public-engineering#190, this setting is removed.

@openedx-webhooks
Copy link

Thanks for the pull request, @Yagnesh1998! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 5, 2023
@Yagnesh1998
Copy link
Contributor Author

@e0d I think all checks have passed.

@e0d e0d requested review from dianakhuang and robrap July 5, 2023 08:10
@e0d
Copy link
Contributor

e0d commented Jul 5, 2023

@robrap and @dianakhuang I believe you were involved in scoping the issue that inspired this work. OSPR is very small. Can one of you review?

@robrap
Copy link
Contributor

robrap commented Jul 5, 2023

@Yagnesh1998: Thank you. Would you mind adding the full link to the DEPR in your commit comment? Providing the ticket link in your commit description (not the title) is probably a good pattern to follow as you approach this work. Thanks again.

alex-sheehan-edx and others added 22 commits July 6, 2023 10:59
* feat: ignore v2 xblocks in independance contract

* docs: add documentation as to failure point
…penedx#32590)

Otherwise we're not really respecting the package-lock file and won't get
repeatable results.

Also:

- Clean up old error handling for npm<3. Were on npm 8 now. Probably
  can get rid of this.
- Use the shorthand `npm ci` rather than `npm clean-install` just for
  consistency with code elsewhere.
- Update comments in tests to be explicit about use of ci rather than
  install
…ation

This part was added in 3d7246e.
Then, 0dd4978 refactored the approach, but
did not remove these lines.
…ional) (openedx#32552)

Plus remove a few unused and indirect dependencies
* feat: add pagination in course enrollment list API

* refactor: enrollment course list API

* refactor: follow best practice in course enrollment list API
Use an existing 3rd party library to generate RST docs from the existing
swagger.yaml file.
* This project was not being published anywhere.
* The previous commit that adds openapi generation capabilities to the
  `docs/guides` replaces what this was doing.
* Swagger was renamed to OpenAPI at some point so use the new name for
  clarity.

* Prefix with `lms` to make it clear that these are the APIs from the
  LMS and may not all be available in the CMS.
Since they were not used anywhere, it is ideal to delete them.

It would be redundant to keep them.
Don't assume that there will be an extra `context` kwarg when using the
bookmark serializer.  We use it this way in the current code but that's
specific to us and not comon to all serializers.  There are a lot of API
documentation tools that automate introspecting serializers but they
won't know  that they have to send in a `context` to the serializer.

To make this serializer behave more like other serializers without
changing the behavior, we just need to check that the `context` value is
defined before we dig into it.  In the case that there is no `context`
we just treat it the same as if there is no `request` in the `context`.
security patch

Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

Co-authored-by: awais786 <[email protected]>
openedx#32645)

* feat: added endpoint for context needed for recommendations experiment

* chore: Removed unnecessary decorator
Yagnesh and others added 8 commits July 6, 2023 10:59
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
* fix: mathjax resize on sindow resize

* fix: add resize for mathjax in wiki and cms
This PR addresses the renaming of the contentstore/xblock_services folder to contentstore/xblock_storage_handlers as a follow-up to PR openedx#32282. The renaming is done to prevent naming conflicts with xblock runtime services and to make the purpose of the files more understandable. The file xblock_service.py has been renamed to view_handlers.py to better reflect its functionality.

Justification and Future Refactoring Outlook:
The xblock_storage_handlers folder contains service methods that implement the business logic for view endpoints located in contentstore/views/block.py. It is renamed to xblock_storage_handlers to reflect its responsibility of handling storage-related operations of xblocks, such as creation, retrieval, and deletion.

The view_handlers.py file includes business methods called by the view endpoints. These methods, such as handle_xblock, delete_orphans, etc., interact with the required modulestore methods, handle any errors, and aggregate and serialize data for the response.

The term 'handler' in the context of 'view_handlers.py' represents methods that facilitate business logic for view endpoints. It is critical to note the distinction between these 'handler methods' and the xblock_handler method. The xblock_handler is a view endpoint itself, residing in contentstore/views/block.py, and is well known in this context. Although its name might suggest otherwise, it is not a handler in the sense of the 'handler methods' we've defined in 'view_handlers.py'. To maintain consistency with existing naming conventions, it remains as xblock_handler.
@Yagnesh1998 Yagnesh1998 requested a review from a team as a code owner July 6, 2023 05:33
@Yagnesh1998
Copy link
Contributor Author

I think there is some mistake I make another DEPR request

@Yagnesh1998 Yagnesh1998 closed this Jul 6, 2023
@openedx-webhooks
Copy link

@Yagnesh1998 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.