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

Remove transcript credentials and preferences view #229

Conversation

zainab-amir
Copy link
Contributor

@zainab-amir zainab-amir commented May 18, 2020

With our decision to move the transcript preferences and credential model to VEM, we no longer need following in edx-val:

  • TranscriptPreferenceView
  • TranscriptCredentialsView
  • TranscriptCredentials model

Ticket: PROD-1597

NOTE:

Currently TranscriptCredentials model has only 21 db records in production

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

Looks good. We can get a review from SRE too, to let them know we are removing this model from VAL. We can re-direct them to VEM ADR if needed.

@DawoudSheraz
Copy link
Contributor

Remember to bump the version in setup.py before merging.

@zainab-amir
Copy link
Contributor Author

Devops Ticket for review: https://openedx.atlassian.net/browse/DOS-1140

Version will be updated in next PR, where we remove the django-fernet-fields from val requirements

Copy link

@adzuci adzuci 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 raising this to SRE. I'm not very familiar with the VAL codebase, but I think this should be fine to merge as one or two PRs and with or without a reverse migration.

operations = [
migrations.DeleteModel(
name='TranscriptCredentials',
),
Copy link

Choose a reason for hiding this comment

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

Do you need to do this in two PRs, one for the code change and one for the migration?

Also, could we add a reverse migration to put this back just in case / to follow best practices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the PR to separate code changes and migration changes

@zainab-amir zainab-amir force-pushed the zamir/PROD-1597/remove_transcript_credentials_and_pref_view branch from 34fe8d0 to 970b699 Compare May 21, 2020 05:51
@zainab-amir zainab-amir force-pushed the zamir/PROD-1597/remove_transcript_credentials_and_pref_view branch from bf368af to 797f9bf Compare May 21, 2020 07:12
@zainab-amir zainab-amir merged commit ea3d68d into master May 21, 2020
@zainab-amir zainab-amir deleted the zamir/PROD-1597/remove_transcript_credentials_and_pref_view branch May 21, 2020 09:08
@awais786 awais786 mentioned this pull request Aug 10, 2023
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.

3 participants