-
Notifications
You must be signed in to change notification settings - Fork 20
[BD-14] fix: bundles list view Request-URI Too Long error #197
[BD-14] fix: bundles list view Request-URI Too Long error #197
Conversation
There could be a lot of bundles, so we can't use query params because it will raise the "Request-URI Too Long" error. Use the request data in such case.
Thanks for the pull request, @dyudyunov! When this pull request is ready, tag your edX technical lead. |
@ormsbee It seems somewhat logical to me that ordinal ids might get too long, but is this solution more future proof than switching to a fixed-length hash? |
Yes, because in some moment there could be a lot of libraries and each library id (block_id) will be used in the query parameter I think that managing the hash length is equal to changing the URL length limit and isn't so flexible. The body length limit is pretty big to not worry about reaching it. |
So I tested this using openedx/edx-platform#30895 as my branch of edx-platform. I was able to perform Create Read Update (no delete because I'm not sure it works) operations on a library locally. With that in mind, I am now happy to thumb this PR and then edx-platform one. What I am interested in, is how we plan to deploy these changes and the order we have considered. I assume it would be would be:
|
Yes, the sequence is correct (as I've described in the edx-platform PR) |
@dyudyunov @connorhaugh What's left to get this PR and the edx-platform one merged? |
All seems to be ready for that 🙂 |
@ormsbee @connorhaugh would you mind reviewing prior to merge if you haven't already? |
Friendly ping on this :) |
@ormsbee Hi Dave - could you please review? |
BD-14 project was closed in the RG and all additional work should be scheduled with our PMs |
@jristau1984 is this PR still relevant? |
Hi @e0d - Yes, this and several related PRs are still relevant. 2U is currently blocked on rolling out this functionality until the Problem Editor works in the course authoring MFE; that editor buildout is currently in flight. The review of this and the related PRs is happening in parallel to that buildout, and we expect to complete all of these reviews in the next few weeks. Please feel free to reach out if there are any questions or concerns! |
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.
@connorhaugh: I'm approving this, but I leave the merge to your discretion.
@dyudyunov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
There could be a lot of bundles, so we can't use query params
because it will raise the "Request-URI Too Long" error. Use the
request data in such a case.