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

Encryption fields added to moodle config #1889

Merged
merged 17 commits into from
Nov 21, 2023
Merged

Encryption fields added to moodle config #1889

merged 17 commits into from
Nov 21, 2023

Conversation

MueezKhan246
Copy link
Contributor

Description:
Added encryption fields to the model of moodle config.

JIRA:
https://2u-internal.atlassian.net/jira/software/c/projects/ENT/issues/ENT-5613

@MueezKhan246 MueezKhan246 force-pushed the MueezKhan/Encryption-Fields-Added-To-MoodleConfig branch 3 times, most recently from 1ab409a to 8d12bb0 Compare October 3, 2023 13:53
@MueezKhan246 MueezKhan246 force-pushed the MueezKhan/Encryption-Fields-Added-To-MoodleConfig branch 2 times, most recently from 48a4adf to 777f04c Compare October 5, 2023 11:25
sameenfatima78
sameenfatima78 reviewed Oct 10, 2023
View reviewed changes
Copy link
Member

@sameenfatima78 sameenfatima78 left a comment

Choose a reason for hiding this comment

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

Left some questions. otherwise looks good!

integrated_channels/moodle/migrations/0028_auto_20230928_1530.py Outdated Show resolved Hide resolved
integrated_channels/moodle/migrations/0028_auto_20230928_1530.py Outdated Show resolved Hide resolved
@MueezKhan246
Merge branch 'master' of https://github.com/openedx/edx-enterprise in…
2c3b179 
…to MueezKhan/Encryption-Fields-Added-To-MoodleConfig
@MueezKhan246 MueezKhan246 force-pushed the MueezKhan/Encryption-Fields-Added-To-MoodleConfig branch from 777f04c to 2c3b179 Compare October 11, 2023 08:34
@MueezKhan246
Copy link
Contributor Author

@johnnagro kindly please review this, whenever you get the time to do so.

sameenfatima78
sameenfatima78 requested changes Nov 1, 2023
View reviewed changes
Copy link
Member

@sameenfatima78 sameenfatima78 left a comment

Choose a reason for hiding this comment

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

Left some comments.

CHANGELOG.rst Outdated Show resolved Hide resolved
integrated_channels/moodle/migrations/0028_auto_20230928_1530.py Outdated Show resolved Hide resolved
integrated_channels/moodle/models.py Outdated Show resolved Hide resolved
integrated_channels/moodle/models.py Outdated Show resolved Hide resolved
integrated_channels/moodle/models.py Outdated Show resolved Hide resolved
integrated_channels/moodle/models.py Outdated Show resolved Hide resolved
integrated_channels/moodle/models.py Outdated Show resolved Hide resolved
integrated_channels/moodle/models.py Outdated Show resolved Hide resolved
integrated_channels/utils.py Outdated Show resolved Hide resolved
sameenfatima78
sameenfatima78 reviewed Nov 13, 2023
View reviewed changes
Copy link
Member

@sameenfatima78 sameenfatima78 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! Just one small suggestion.

integrated_channels/moodle/models.py Outdated Show resolved Hide resolved
sameenfatima78
sameenfatima78 reviewed Nov 15, 2023
View reviewed changes
Copy link
Member

@sameenfatima78 sameenfatima78 left a comment

Choose a reason for hiding this comment

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

Left some suggestions but looks good overall!

CHANGELOG.rst Outdated Show resolved Hide resolved
integrated_channels/moodle/migrations/0028_auto_20230928_1530.py Show resolved Hide resolved
integrated_channels/moodle/migrations/0030_auto_20231114_1509.py Outdated Show resolved Hide resolved
sameenfatima78
sameenfatima78 approved these changes Nov 19, 2023
View reviewed changes
Copy link
Member

@sameenfatima78 sameenfatima78 left a comment

Choose a reason for hiding this comment

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

LGTM

@hamzawaleed01
Copy link
Member

@MueezKhan246 , @sameenfatima78

I've got some question just for my understanding

  1. Do we need to add stuff like this and this in upcoming PRs 🤔
  2. Once the data is migrated, @MueezKhan246 will create another PR to delete the unencrypted columns?
  3. Once the 2nd point is done ☝️, Mueez will have to make sure that in those API endpoints that are fetching Moodle config, we're sending unencrypted data right? 🤔

@MueezKhan246
Copy link
Contributor Author

@MueezKhan246 , @sameenfatima78

I've got some question just for my understanding

  1. Do we need to add stuff like this and this in upcoming PRs 🤔
  2. Once the data is migrated, @MueezKhan246 will create another PR to delete the unencrypted columns?
  3. Once the 2nd point is done ☝️, Mueez will have to make sure that in those API endpoints that are fetching Moodle config, we're sending unencrypted data right? 🤔

@hamzawaleed01 yes, all these steps will be implemented in upcoming PRs
cc. @sameenfatima78

@MueezKhan246 MueezKhan246 merged commit 0858964 into master Nov 21, 2023
8 checks passed
@MueezKhan246 MueezKhan246 deleted the MueezKhan/Encryption-Fields-Added-To-MoodleConfig branch November 21, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sameenfatima78 sameenfatima78 sameenfatima78 approved these changes

@zamanafzal zamanafzal Awaiting requested review from zamanafzal

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

Successfully merging this pull request may close these issues.
4 participants
@MueezKhan246 @hamzawaleed01 @zamanafzal @sameenfatima78