Skip to content
This repository was 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
connorhaugh merged 1 commit into
openedx-unsupported:masterfrom
raccoongang:fix-bundles-api
Apr 25, 2023
Merged

[BD-14] fix: bundles list view Request-URI Too Long error#197
connorhaugh merged 1 commit into
openedx-unsupported:masterfrom
raccoongang:fix-bundles-api

Conversation

@dyudyunov

Copy link
Copy Markdown
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

openedx-webhooks commented Aug 24, 2022

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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

connorhaugh commented Sep 30, 2022

Copy link
Copy Markdown
Contributor

So I tested this using openedx/openedx-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/openedx-platform#30895, wait for it to deploy
  2. merge this PR, deploy it. Am I correct?

@dyudyunov

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@dyudyunov

Copy link
Copy Markdown
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
Copy Markdown

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

@mphilbrick211

Copy link
Copy Markdown

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

Friendly ping on this :)

@mphilbrick211

Copy link
Copy Markdown

@ormsbee Hi Dave - could you please review?

@dyudyunov

Copy link
Copy Markdown
Contributor Author

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

@e0d

e0d commented Jan 6, 2023

Copy link
Copy Markdown

@jristau1984 is this PR still relevant?

@jristau1984

Copy link
Copy Markdown
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!

@ormsbee ormsbee left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown

@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