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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Oct 24, 2023
Merged

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

merged 3 commits into from
Oct 24, 2023

Conversation

farhan
Copy link
Contributor

@farhan farhan commented Oct 11, 2023 •
edited by Agrendalath
Loading

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

@farhan farhan mentioned this pull request Oct 11, 2023
Closed
Agrendalath
Agrendalath requested changes Oct 11, 2023
View reviewed changes
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
Closed
salman2013
salman2013 suggested changes Oct 17, 2023
View reviewed changes
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 •
edited
Loading

@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 •
edited
Loading

@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 •
edited
Loading

@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

Agrendalath
Agrendalath requested changes Oct 19, 2023
View reviewed changes
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
@farhan
feat!: Remove bok-choy and xblockutils packages
cba7dca 
* 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

Agrendalath
Agrendalath approved these changes Oct 24, 2023
View reviewed changes
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
@Agrendalath Agrendalath mentioned this pull request Jan 9, 2024
Closed
@farhan farhan mentioned this pull request Aug 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Agrendalath Agrendalath Agrendalath approved these changes

@salman2013 salman2013 salman2013 requested changes

@feanil feanil Awaiting requested review from feanil

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove xblockutils package Remove bok-choy usage
4 participants
@farhan @Agrendalath @salman2013 @feanil