Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

[BD-14] fix: bundles list view Request-URI Too Long error #197

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

dyudyunov
Copy link
Contributor

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.

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.
@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Aug 24, 2022
@openedx-webhooks
Copy link

openedx-webhooks commented Aug 24, 2022

Thanks for the pull request, @dyudyunov!

When this pull request is ready, tag your edX technical lead.

@dyudyunov dyudyunov changed the title [BD-14] fix: bundles list view Request-URI Too Long error [WIP][BD-14] fix: bundles list view Request-URI Too Long error Aug 24, 2022
@dyudyunov dyudyunov changed the title [WIP][BD-14] fix: bundles list view Request-URI Too Long error [BD-14] fix: bundles list view Request-URI Too Long error Aug 24, 2022
@connorhaugh
Copy link
Contributor

@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?

@dyudyunov
Copy link
Contributor Author

@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 uuid

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.

@connorhaugh
Copy link
Contributor

connorhaugh commented Sep 30, 2022

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:

  1. merge [BD-14] feat: implement V2 libraries usage for library content block openedx/edx-platform#30895, wait for it to deploy
  2. merge this PR, deploy it. Am I correct?

@dyudyunov
Copy link
Contributor Author

  1. merge [BD-14] feat: implement V2 libraries usage for library content block edx-platform#30895, wait for it to deploy
  2. merge this PR, deploy it. Am I correct?

Yes, the sequence is correct (as I've described in the edx-platform PR)

@bradenmacdonald
Copy link
Contributor

@dyudyunov @connorhaugh What's left to get this PR and the edx-platform one merged?

@dyudyunov
Copy link
Contributor Author

@dyudyunov @connorhaugh What's left to get this PR and the edx-platform one merged?

All seems to be ready for that 🙂

@mphilbrick211
Copy link

@ormsbee @connorhaugh would you mind reviewing prior to merge if you haven't already?

@mphilbrick211
Copy link

@ormsbee @connorhaugh would you mind reviewing prior to merge if you haven't already?

Friendly ping on this :)

@mphilbrick211
Copy link

@ormsbee Hi Dave - could you please review?

@dyudyunov
Copy link
Contributor Author

BD-14 project was closed in the RG and all additional work should be scheduled with our PMs

@e0d
Copy link

e0d commented Jan 6, 2023

@jristau1984 is this PR still relevant?

@jristau1984
Copy link
Contributor

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!

Copy link
Collaborator

@ormsbee ormsbee left a 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.

@connorhaugh connorhaugh merged commit a2c2259 into openedx-unsupported:master Apr 25, 2023
@openedx-webhooks
Copy link

@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.

@dyudyunov dyudyunov deleted the fix-bundles-api branch April 26, 2023 07:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blended PR is managed through 2U's blended developmnt program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants