Skip to content

Consolidate two different implementations of serializing an XBlock to OLX#32115

Merged
bradenmacdonald merged 1 commit intoopenedx:masterfrom
open-craft:block-serializer-consolidate
Apr 24, 2023
Merged

Consolidate two different implementations of serializing an XBlock to OLX#32115
bradenmacdonald merged 1 commit intoopenedx:masterfrom
open-craft:block-serializer-consolidate

Conversation

@bradenmacdonald
Copy link
Contributor

@bradenmacdonald bradenmacdonald commented Apr 21, 2023

Description

Ticket: openedx/modular-learning#54

This is just a refactor to make some code that I've been working on more DRY. i.e. it's removing some minor technical debt by consolidating two very similar classes that are used to serialize individual XBlocks to OLX - one that uses "normal" OLX and one that uses a blockstore-specific variant.

Or at least, that was the plan - I actually found some minor inconsistencies in the API when adding more tests for the new combined API, so it does include some bugfixes too. Mainly, it now always includes url_name in the exported OLX except when in "blockstore mode"; before this it was inconsistent.

Supporting information

See #31904 which introduced the duplication. I split this fix out into a separate PR so that the refactor can be evaluated on its own.

Testing instructions

Check that the test cases provide coverage and the copy-paste feature is still working.

Deadline

None

Other information

Private ref. MNG-3636

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core committer labels Apr 21, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Apr 21, 2023

Thanks for the pull request, @bradenmacdonald!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

@bradenmacdonald bradenmacdonald force-pushed the block-serializer-consolidate branch 2 times, most recently from a5f38c1 to e748c78 Compare April 21, 2023 01:34
@bradenmacdonald bradenmacdonald changed the title Consolidate two different implementations of serializing an XBlock to OLX (WIP) Consolidate two different implementations of serializing an XBlock to OLX Apr 21, 2023
@bradenmacdonald
Copy link
Contributor Author

@Agrendalath This is ready for review.

FYI @ormsbee - in case you want to review, up to you.

@bradenmacdonald bradenmacdonald force-pushed the block-serializer-consolidate branch from e748c78 to 6d01fbc Compare April 21, 2023 19:45
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

👍

  • I tested this: checked that the OLX content generated by the "Copy" functionality is identical (except for adding the url_name)
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

@bradenmacdonald bradenmacdonald merged commit dd927c7 into openedx:master Apr 24, 2023
@bradenmacdonald bradenmacdonald deleted the block-serializer-consolidate branch April 24, 2023 19:16
@openedx-webhooks
Copy link

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core committer open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants