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

[DEPR]: lms/djangoapps/badges #33186

Merged
merged 1 commit into from
Oct 20, 2023
Merged

[DEPR]: lms/djangoapps/badges #33186

merged 1 commit into from
Oct 20, 2023

Conversation

thezaeemaanwar
Copy link
Contributor

@thezaeemaanwar thezaeemaanwar commented Sep 6, 2023 •
edited
Loading

Description

This is phase 1 of Badges deprecation process. The following modifications have been made:

  • The ENABLE_OPENBADGES switch is removed from settings.FEATURES.
  • The accomplishments_shared field has been removed since it only indicated if the badges are public or not.
  • UI for badges within a learner's profile has been removed.
  • Badges have been removed from the certificates app.

UI Change

With badges disabled, there is no visible difference in the UI.
Before:
image
After:
image

Related Links

Issue: #31541
Ticket: openedx/axim-engineering#852

@thezaeemaanwar thezaeemaanwar changed the title Remove badges app feat!: remove badges app Sep 6, 2023
@feanil
Copy link
Contributor

feanil commented Sep 19, 2023

@thezaeemaanwar looking at the failing test case (lms/djangoapps/certificates/tests/test_webview_views.py::CertificateEventTests::test_evidence_event_sent ) it looks that test is specific to the badge events and so can be safely removed without impacting the rest of the certificates code.

Looking at the rest of the file, it looks like there is more Badges related testing in this test file. You'll have to be careful but I think it's safe to remove anything that is testing only the impact of certs on Badges. Some test cases are using DDT to test badges sometimes but not others so you'll have to make sure that the non badge test cases are still retained.

For example, test_rendering_maximum_data tests many things including badges so you'll need to remove just the badges related assertions and code but retain the rest of the test.

@thezaeemaanwar thezaeemaanwar force-pushed the remove_badges_app branch 3 times, most recently from 3381019 to b6656bb Compare October 4, 2023 11:28
@thezaeemaanwar thezaeemaanwar force-pushed the remove_badges_app branch 2 times, most recently from 8a3ecf9 to cab920e Compare October 4, 2023 14:49
thezaeemaanwar
thezaeemaanwar commented Oct 5, 2023
View reviewed changes
openedx/core/djangoapps/user_api/accounts/tests/test_views.py
@@ -359,7 +359,7 @@ class TestAccountsAPI(FilteredQueryCountMixin, CacheIsolationTestCase, UserAPITe

ENABLED_CACHES = ['default']
TOTAL_QUERY_COUNT = 24
FULL_RESPONSE_FIELD_COUNT = 30
FULL_RESPONSE_FIELD_COUNT = 29
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: Since the accomplishments_shared field is removed from the data

@thezaeemaanwar thezaeemaanwar marked this pull request as ready for review October 5, 2023 06:03
farhan
farhan requested changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@farhan farhan left a comment

Choose a reason for hiding this comment

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

It will be great if you could share the screen shots of the UI changes

lms/djangoapps/certificates/tests/test_webview_views.py Show resolved Hide resolved
@thezaeemaanwar thezaeemaanwar force-pushed the remove_badges_app branch 2 times, most recently from ac7360e to 552b20d Compare October 11, 2023 06:20
@thezaeemaanwar thezaeemaanwar changed the title feat!: remove badges app [DEPR]: lms/djangoapps/badges Oct 17, 2023
@feanil
Copy link
Contributor

feanil commented Oct 17, 2023

@thezaeemaanwar look like there is a conflict to resolve. What else is left to do before we can merge this change?

@thezaeemaanwar
Copy link
Contributor Author

@feanil Just the UI screenshots of learner's profile in the description are left.
I'm facing issue in setting up badges on master.

@feanil feanil linked an issue Oct 17, 2023 that may be closed by this pull request
[DEPR]: lms/djangoapps/badges #31541
Closed
@thezaeemaanwar thezaeemaanwar force-pushed the remove_badges_app branch 2 times, most recently from 8aaa159 to 7a768ff Compare October 18, 2023 07:39
@thezaeemaanwar
feat!: Removed Badges App
0b3e4d7 
fix: restored badges handlers

feat: remove FE code for badges

fix: resolved failing tests

fix: removed test case for badges app

fix: unused import error

fix: Response Field Count

fix: shareable account response length

fix: resolved PR comments

fix: revert settings override

feat!: Removed Badges App

fix: restored badges handlers

feat: remove FE code for badges

fix: resolved failing tests

fix: removed test case for badges app

fix: unused import error

fix: Response Field Count

fix: shareable account response length

fix: revert subscription badge
@hurtstotouchfire hurtstotouchfire added the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Oct 18, 2023
@feanil
Copy link
Contributor

feanil commented Oct 19, 2023

This looks good to me. I'll merge tomorrow morning when I can monitor its deploy to edx.org

@feanil feanil merged commit 2e75ced into openedx:master Oct 20, 2023
61 checks passed
@edx-pipeline-bot
Copy link
Contributor

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

@rgraber
Copy link
Contributor

rgraber commented Oct 20, 2023

@feanil is it possible that we're missing a CLA for this? The commit is failing CI checks 2e75ced

@feanil
Copy link
Contributor

feanil commented Oct 20, 2023

No, we definitely have a CLA for this person, not sure why it would fail

@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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

2U Release Notice: This PR has been deployed to the edX production environment.

@thezaeemaanwar thezaeemaanwar mentioned this pull request Oct 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@farhan farhan farhan approved these changes

@feanil feanil feanil approved these changes

Assignees
No one assigned
Labels
needs maintainer attention Issue or PR specifically needs the attention of the maintainer.
Projects
Aperture-Maintained
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DEPR]: lms/djangoapps/badges
6 participants
@thezaeemaanwar @feanil @edx-pipeline-bot @rgraber @farhan @hurtstotouchfire