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

feat: removing the application stage 2 #35186

Conversation

deborahgu
Copy link
Member

@deborahgu deborahgu commented Jul 25, 2024

This removes the deprecated demographics django application. No other apps in the repository currently reference this djangoapp in code or tables.

This DEPR was accepted in #35127. Only 2U ever used this app.

  • removing the application folder
  • removing references in all other files

FIXES: APER-3560

* removing the application folder
* removing references in all other files

FIXES: APER-3560
@deborahgu deborahgu added the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Jul 25, 2024
removing an entire app.

FIXES: APER-3560
actions need to know about changes to djangoapps

FIXES: APER-3560
@deborahgu deborahgu marked this pull request as ready for review July 25, 2024 21:40
@@ -474,28 +474,6 @@ def replace_usernames(self, username_mappings):
return self._request('POST', api_url, json=data)


class DemographicsApi(BaseApiClient):
Copy link
Contributor

Choose a reason for hiding this comment

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

This particular endpoint wasn't part of the Demographics app in edx-platform... wouldn't we still need to support retiring learners in the Demographics IDA itself?

I don't know enough about the inner workings of the retirement pipeline yet to know if it's using this particular piece of code. It appears that the DemographicsAPI is still part of the (forked) tubular repo in our GitHub org... so maybe we're okay removing this one?

Do you have a better understanding of the spaghetti?

Copy link
Member Author

Choose a reason for hiding this comment

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

Aren't we about to retire the whole IDA, though? Does it matter if we miss a couple of retirement requests in the window before we delete all the data?

Copy link
Member Author

Choose a reason for hiding this comment

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

on second thought, Justin is right, the DEPR of the IDA is soon but it's not imminent, better safe than sorry.

undoing a removal from the retirement pipeline that we aren't quite ready for yet

FIXES: APER-3560
Copy link
Contributor

@justinhynes justinhynes left a comment

Choose a reason for hiding this comment

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

Thanks for talking through the changes! LGTM.

@deborahgu deborahgu merged commit 089b34a into master Jul 26, 2024
49 checks passed
@deborahgu deborahgu deleted the dkaplan1/APER-3560_depr-remove-demographics-django-app-from-edx-platform-pt2 branch July 26, 2024 15:13
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs maintainer attention Issue or PR specifically needs the attention of the maintainer.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants