Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Remove bok-choy and xblockutils (use xblock.utils instead) packages #357

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

farhan
Copy link
Contributor

@farhan farhan commented Oct 11, 2023

Description

  1. Removed xblock utils package and migrated to xblock package utils:
  2. Removed bok-choy packages:
  3. Updated python requirements

Breaking change in tests

There is no breaking change in the xblock feature in the implementation of this PR
As we have parted ways with the bok-choy library this PR is removing all the integrating test cases along with the base test classes because they are linked with the library.

Notes

Resolves #356
Resolves #354

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.

@farhan, this should define a minimum version for the XBlock dependency in the requirements/base.in.

We should bump the major version here if we do not provide the fallback import from xblockutils. We should also clearly state which of the edx-platform named releases are compatible with this new version.

@Agrendalath
Copy link
Member

@farhan, why are we removing integration tests? Are we going to replace them with something else?

@farhan
Copy link
Contributor Author

farhan commented Oct 13, 2023

@farhan, why are we removing integration tests? Are we going to replace them with something else?

@Agrendalath We are removing bok-choy integration tests because the library has been deprecated.

Bok-choy integration test has not been copied from xblock-utils to xblock/tests/utils

due to which the repositories that are using the base test classes of xblock/integration/tests are breaking.

We are already planning to remove bok-choy test cases, so covering it in this pr as well.
relevant ticket: #354

@farhan farhan changed the title [WIP] Remove xblockutil [WIP] Remove bok-choy and xblockutils package Oct 16, 2023
@farhan farhan changed the title [WIP] Remove bok-choy and xblockutils package Removed bok-choy and xblockutils package Oct 17, 2023
@farhan farhan mentioned this pull request Oct 17, 2023
tests/integration/test_events.py Show resolved Hide resolved
requirements/test.in Outdated Show resolved Hide resolved
@salman2013
Copy link
Contributor

@farhan few test cases are failing like
https://github.com/openedx/xblock-drag-and-drop-v2/actions/runs/6545659095/job/17774598693?pr=357

@farhan
Copy link
Contributor Author

farhan commented Oct 17, 2023

@Agrendalath
I have completed my implementation.
I have removed all the things related to the integration test as all of them are linked to the Bok-choy test cases library.
I have removed the relevant configuration and docs related to integration tests as well. Please guide should we remove all of it as we don't have any plan in the pipeline to write integration tests again using other library.

We should also clearly state which of the edx-platform named releases are compatible with this new version.

I have added the details in the change log, should we state this change on some other place as well?

@farhan farhan requested a review from feanil October 17, 2023 12:35
@Agrendalath
Copy link
Member

Agrendalath commented Oct 18, 2023

@farhan

I have removed all the things related to the integration test as all of them are linked to the Bok-choy test cases library.
I have removed the relevant configuration and docs related to integration tests as well. Please guide should we remove all of it as we don't have any plan in the pipeline to write integration tests again using other library.

What do you mean by "all of it"?

I have added the details in the change log, should we state this change on some other place as well?

This is a breaking change, so we should indicate this in the commit, PR's name + description, and the changelog. Please take a look at #318 for an example of how we have been doing it.

Also, I wonder if xblock[django]==1.8.1 really is incompatible with older named releases. I don't see any breaking changes in its changelog or release notes.

@farhan
Copy link
Contributor Author

farhan commented Oct 18, 2023

What do you mean by "all of it"?

@Agrendalath I want to ask please verify all the removal done in this PR seems right to you

Also, I wonder if xblock[django]==1.8.1 really is incompatible with older named releases. I don't see any breaking changes in its changelog or release notes.

That's true there is no breaking change in xblock release v1.8.0 and v1.8.1

@farhan farhan changed the title Removed bok-choy and xblockutils package feat! Removed bok-choy and xblockutils (use xblock.utils instead) packages Oct 19, 2023
@farhan
Copy link
Contributor Author

farhan commented Oct 19, 2023

@Agrendalath
PR (Pull Request) is now ready for your review. I have also made necessary updates to the description, specifically addressing the breaking change. Your prompt review of these changes would be greatly appreciated.
cc: @feanil

drag_and_drop_v2/drag_and_drop_v2.py Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
requirements/base.in Outdated Show resolved Hide resolved
requirements/workbench.in Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
* Removed xblock-utils package: Replace xblockutils.* imports with xblock.utils.* imports
* Removed bok-choy package and its relevant test cases
@farhan
Copy link
Contributor Author

farhan commented Oct 24, 2023

@Agrendalath Addressed all the requested changes.
PR is available for next pass

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.

@farhan, thanks for working on this!

👍

  • I tested this: checked that the XBlock is working correctly with Nutmeg and master versions of the devstack
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a

@Agrendalath Agrendalath changed the title feat! Removed bok-choy and xblockutils (use xblock.utils instead) packages feat: Remove bok-choy and xblockutils (use xblock.utils instead) packages Oct 24, 2023
@Agrendalath Agrendalath changed the title feat: Remove bok-choy and xblockutils (use xblock.utils instead) packages refactor: Remove bok-choy and xblockutils (use xblock.utils instead) packages Oct 24, 2023
@Agrendalath Agrendalath merged commit fc54d87 into openedx:master Oct 24, 2023
7 checks passed
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.

Remove xblockutils package Remove bok-choy usage
4 participants