feanil/common constraint backport #37798
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Course Info Blocks API endpoint has been known to be rather slow to return the response. Previous investigation showed that the major time sink was the get_course_blocks function, which is called three times in a single request. This commit aims to improve the response times by reducing the number of times that this function is called. Solution Summary The first time the function get_course_blocks is called, the result (transformed course blocks) is stored in the current WSGI request object. Later in the same request, before the second get_course_blocks call is triggered, the already transformed course blocks are taken from the request object, and if they are available, get_course_blocks is not called (if not, it is called as a fallback). Later in the request, the function is called again as before (see Optimization Strategy and Difficulties). Optimization Strategy and Difficulties The original idea was to fetch and transform the course blocks once and reuse them in all three cases, which would reduce get_course_blocks call count to 1. However, this did not turn out to be a viable solution because of the arguments passed to get_course_blocks. Notably, the allow_start_dates_in_future boolean flag affects the behavior of StartDateTransformer, which is a filtering transformer modifying the block structure returned. The first two times allow_start_dates_in_future is False, the third time it is True. Setting it to True in all three cases would mean that some blocks would be incorrectly included in the response. This left us with one option - optimize the first two calls. The difference between the first two calls is the non-filtering transformers, however the second call applies a subset of transformers from the first call, so it was safe to apply the superset of transformers in both cases. This allowed to reduce the number of function calls to 2. However, the cached structure may be further mutated by filters downstream, which means we need to cache a copy of the course structure (not the structure itself). The copy method itself is quite heavy (it calls deepcopy three times), making the benefits of this solution much less tangible. In fact, another potential optimization that was considered was to reuse the collected block structure (pre-transformation), but since calling copy on a collected structure proved to be more time-consuming than calling get_collected, this change was discarded, considering that the goal is to improve performance. Revised Solution To achieve a more tangible performance improvement, it was decided to modify the previous strategy as follows: * Pass a for_blocks_view parameter to the get_blocks function to make sure the new caching logic only affects the blocks view. * Collect and cache course blocks with future dates included. * Include start key in requested fields. * Reuse the cached blocks in the third call, which is in get_course_assignments * Before returning the response, filter out any blocks with a future start date, and also remove the start key if it was not in requested fields
fix: validation API for certificates
This pulls in publishing dependency changes from: openedx/openedx-learning#369 This fixes a bug where publishing a Content Library v2 container would publish only its direct children instead of publishing all ancestors. Backports: 190a8b8 Co-authored-by: Kyle McCormick <[email protected]>
Signed-off-by: Farhaan Bukhsh <[email protected]>
) (#37629) Calls `LIBRARY_CONTAINER_PUBLISHED` when publishing a container that is child of another container.
fix: validation API for certificates (backport - Ulmo)
…cement points (#37633) * feat: filter libraries based on user-role scopes (#37564) (cherry picked from commit 6c6fc5d) * feat: add openedx-authz to user_can_create_library and require_permission_for_library_key (#37501) * feat: add the authz check to the library api function feat: add the authz publish check in rest_api blocks and containers feat: add the authz checks in libraries and refactor feat: add collections checks feat: update enforcement in serializer file refactor: refactor the permission check functions fix: fix value error fix: calling the queries twice * test: add structure for test and apply feedback refactor: refactor the tests and apply feedback fix: apply feedback Revert "refactor: refactor the tests and apply feedback" This reverts commit aa0bd52. refactor: use constants and avoid mapping test: fix the test to have them in order docs: about we rely on bridgekeeper and the old check for two cases docs: update openedx/core/djangoapps/content_libraries/api/libraries.py Co-authored-by: Maria Grimaldi (Majo) <[email protected]> refactor: use global scope wildcard instead of * refactor: allow receiving PermissionData objects refactor: do not inherit from BaseRolesTestCase to favor CL setup methods If both BaseRolesTestCase and ContentLibrariesRestApiTest define a method with the same name (e.g., setUp()), Python will use the one found first in the MRO, which is the one in BaseRolesTestCase because it is listed first in the class definition leading to unexpected behavior. refactor: remove unnecessary imports and indent * chore: bump openedx-authz version (cherry picked from commit f4f14a6) * feat: Upgrade Python dependency openedx-authz (#37652) * feat: Upgrade Python dependency openedx-authz handle cache invalidation Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` * fix: update the num of queries in tests --------- Co-authored-by: MaferMazu <[email protected]> Co-authored-by: Maria Fernanda Magallanes Zubillaga <[email protected]> (cherry picked from commit 122b4e0) * chore: update requirements to fix the inconsistency --------- Co-authored-by: Maria Grimaldi (Majo) <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was originally permitted for forum moderators and course staff as a way to fight spam, but it was decided that this functionality was too dangerous to open up that widely. There are some tentative plans around how to make this a more fully supported feature, but until then, we're restricting this to global staff on the Ulmo release branch as an interim measure. The frontend was already disabled in the Ulmo release, meaning that this will be a backend-only API (i.e. if you really know what you're doing and absolutely need this functionality). The assumption is that this feature will continue to be developed on the master branch and will be in better shape for Verawood.
…) (#37679) - Fix the issue described in openedx/frontend-app-authoring#2626 - Publish components and containers after migrate (cherry picked from commit b9e5683)
…7688) Prevents notification failures with MySQL backend by ensuring signals are only sent after database transactions commit. This fixes race conditions where Celery workers couldn't see newly created threads. - Added send_signal_after_commit() helper function - Updated both thread creation paths to use the helper Co-authored-by: Taimoor Ahmed <[email protected]>
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
* fix: Course search pill not cleared when text deleted * chore: fix spacing Co-authored-by: Feanil Patel <[email protected]> --------- Co-authored-by: Feanil Patel <[email protected]>
There is no way to resume either the backup or restore library actions, i.e. if you navigate away from it, you have to do it again. This is a limitation of the current UI because we wanted to get something quick and simple in for Ulmo, but it also reflects the fact that library backup/restore should be much faster than course import/export has historically been. In any case, sending an email for a 5-10 second task is unnecessary and distracting, so this commit suppresses the email. Note: I'm using local imports to get around the fact that the content_libraries public API is used by content_libraries/tasks.py which defines the tasks. I can't import from content_libraries/tasks.py directly, because that would violate import linter rules forbidding other apps from importing things outside of api.py. This isn't ideal, but it keeps the fix small and it keeps the logic in the content_libraries app.
We previously fixed this when the CourseLimitedStaffRole was applied to a course but did not handle the case where the role is applied to a user for a whole org. The underlying issue is that the CourseLimitedStaffRole is a subclass of the CourseStaffRole and much of the system assumes that subclesses are for giving more access not less access. To prevent that from happening for the case of the CourseLimitedStaffRole, when we do CourseStaffRole access checks, we use the strict_role_checking context manager to ensure that we're not accidentally granting the limited_staff role too much access.
The "overview" and "about_sidebar_html" fields in the
CoursewareInformation view (/api/courseware/course/{courseId}) were
returning unsanitized HTML and relying on the client to sanitize it.
This commit shifts that work to the server side (clean_dangerous_html)
to remove potentially dangerous tags when generating the response. The
source of this data is modified in the "Settings and Details" section
of a course in Studio.
fix: CourseLimitedStaffRole should not be able to access studio.
Prior to this, if ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES was enabled, we would not return the orgs that someone had course creator rights on, even if ENABLE_CREATOR_GROUP was enabled. (For the moment, we are conflating "can create courses" with "can create libraries" for a given org, even though we should probably eventually split those apart.)
Re-compilation and upgrade-package should be able to run without updating the common_constraints.txt file. We do this all the time when backporting fixes to older releases. We shouldn't pull in the latest common_constraints.txt in those cases as they may not be compatible with older releases.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LIBRARY_CONTAINER_PUBLISHEDfor parent of containers (fix: CallLIBRARY_CONTAINER_PUBLISHEDfor parent of containers #37622) ([Backport] fix: CallLIBRARY_CONTAINER_PUBLISHEDfor parent of containers (#37… #37629)