Skip to content

fix: use new xblock handler URLs for sequence blocks - #423

Merged
mikix merged 1 commit into
masterfrom
mikix/new-handler-urls
May 6, 2021
Merged

fix: use new xblock handler URLs for sequence blocks#423
mikix merged 1 commit into
masterfrom
mikix/new-handler-urls

Conversation

@mikix

@mikix mikix commented Apr 26, 2021

Copy link
Copy Markdown
Contributor

They've changed to proper new-style handlers, so the URL also changed. This will let us get the fix for green checkmarks showing up as audit users pass FBE units, even though those units aren't actually complete.

https://openedx.atlassian.net/browse/AA-409
https://github.com/edx/edx-platform/pull/27422

@Dillon-Dumesnil Dillon-Dumesnil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subject to approval of the backend, but all these changes look good.

Comment thread src/courseware/data/api.js Outdated
export async function getBlockCompletion(courseId, sequenceId, usageKey) {
// Post data sent to this endpoint must be url encoded
// TODO: Remove the need for this to be the case.
// TODO: Ensure this usage of URLSearchParams is working in Internet Explorer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Wonder whatever happened here.

urlEncoded.toString(),
requestConfig,
`${getSequenceHandlerUrl(courseId, sequenceId)}/goto_position`,
// Position is 1-indexed on the server and 0-indexed in this app. Adjust here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:chef_kiss:

@MatthewPiatetsky MatthewPiatetsky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

were you able to test this locally?

@mikix

mikix commented Apr 26, 2021

Copy link
Copy Markdown
Contributor Author

were you able to test this locally?

Yeah with the new backend, both master and this PR worked as expected (master had the bug but was able to call old handlers fine, this PR called new fixed handlers). But this needs the new backend code to be in prod first.

They've changed to proper new-style handlers, so the URL also
changed. This will let us get the fix for green checkmarks showing
up as audit users pass FBE units, even though those units aren't
actually complete.

AA-409
@mikix
mikix force-pushed the mikix/new-handler-urls branch from b3415de to 32b586b Compare April 27, 2021 15:13
@mikix
mikix marked this pull request as ready for review April 27, 2021 15:16
@codecov

codecov Bot commented Apr 27, 2021

Copy link
Copy Markdown

Codecov Report

Merging #423 (32b586b) into master (43aa629) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
- Coverage   82.39%   82.37%   -0.02%     
==========================================
  Files         195      195              
  Lines        3113     3104       -9     
  Branches      774      773       -1     
==========================================
- Hits         2565     2557       -8     
+ Misses        532      531       -1     
  Partials       16       16              
Impacted Files Coverage Δ
...are/data/__factories__/sequenceMetadata.factory.js 69.56% <ø> (-1.27%) ⬇️
src/courseware/data/api.js 98.76% <100.00%> (+1.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43aa629...32b586b. Read the comment docs.

@mikix
mikix merged commit e46977f into master May 6, 2021
@mikix
mikix deleted the mikix/new-handler-urls branch May 6, 2021 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants