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: send group membership invite and remove emails #2049

Merged
merged 23 commits into from
Apr 18, 2024

Conversation

katrinan029
Copy link
Contributor

@katrinan029 katrinan029 commented Mar 15, 2024

Add tasks for group membership invites and removal. The reminder emails will be done in enterprise-access

Braze templates:

https://2u-internal.atlassian.net/browse/ENT-8504

Merge checklist:

  • Any new requirements are in the right place (do not manually modify the requirements/*.txt files)
    • base.in if needed in production but edx-platform doesn't install it
    • test-master.in if edx-platform pins it, with a matching version
    • make upgrade && make requirements have been run to regenerate requirements
  • make static has been run to update webpack bundling if any static content was updated
  • ./manage.py makemigrations has been run
    • Checkout the Database Migration Confluence page for helpful tips on creating migrations.
    • Note: This must be run if you modified any models.
      • It may or may not make a migration depending on exactly what you modified, but it should still be run.
    • This should be run from either a venv with all the lms/edx-enterprise requirements installed or if you checked out edx-enterprise into the src directory used by lms, you can run this command through an lms shell.
      • It would be ./manage.py lms makemigrations in the shell.
  • Version bumped
  • Changelog record added
  • Translations updated (see docs/internationalization.rst but also this isn't blocking for merge atm)

Post merge:

  • Tag pushed and a new version released
    • Note: Assets will be added automatically. You just need to provide a tag (should match your version number) and title and description.
  • After versioned build finishes in GitHub Actions, verify version has been pushed to PyPI
    • Each step in the release build has a condition flag that checks if the rest of the steps are done and if so will deploy to PyPi.
      (so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
  • PR created in edx-platform to upgrade dependencies (including edx-enterprise)
    • This must be done after the version is visible in PyPi as make upgrade in edx-platform will look for the latest version in PyPi.
    • Note: the edx-enterprise constraint in edx-platform must also be bumped to the latest version in PyPi.

Copy link
Contributor

@alex-sheehan-edx alex-sheehan-edx left a comment

Choose a reason for hiding this comment

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

Generally, the right direction. I think we need to think about which membership records we're pulling from the DB and we can simplify down some of the resulting logic given we have access to the pending learner record

@katrinan029 katrinan029 force-pushed the knguyen2/ent-8504 branch 3 times, most recently from 99d85cf to cca442b Compare March 21, 2024 17:48
Copy link
Contributor

@alex-sheehan-edx alex-sheehan-edx left a comment

Choose a reason for hiding this comment

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

Looks great so far- one comment on filtering for only pending learners in group and unit tests but otherwise on the right track 👍

enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/serializers.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
@katrinan029 katrinan029 force-pushed the knguyen2/ent-8504 branch 2 times, most recently from a422304 to 12481f5 Compare March 27, 2024 19:31
feat: send group membership invite, remove, and reminder emails

feat: send group membership invite, remove, and reminder emails

feat: send group membership invite, remove, and reminder emails

fix: file name

chore: refactored

chore: refactored

chore: refactored sending group reminder email to realized learners

fix: deleted unused file

chore: update

fix: updated tests

chore: removed comment

chore: removed empty line
@katrinan029 katrinan029 changed the title feat: send group membership invite, remove, and reminder emails feat: send group membership invite and remove emails Mar 27, 2024
@katrinan029 katrinan029 marked this pull request as ready for review March 27, 2024 19:36
@katrinan029 katrinan029 requested a review from a team March 27, 2024 19:36
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/tasks.py Outdated Show resolved Hide resolved
enterprise/tasks.py Outdated Show resolved Hide resolved
enterprise/tasks.py Outdated Show resolved Hide resolved
enterprise/tasks.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
tests/test_enterprise/api/test_views.py Outdated Show resolved Hide resolved
@katrinan029 katrinan029 requested a review from iloveagent57 April 2, 2024 15:05
Copy link
Contributor

@alex-sheehan-edx alex-sheehan-edx left a comment

Choose a reason for hiding this comment

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

Some thoughts!

enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
tests/test_enterprise/api/test_views.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/tasks.py Show resolved Hide resolved
enterprise/api/v1/serializers.py Outdated Show resolved Hide resolved
enterprise/tasks.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kiram15 kiram15 left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@iloveagent57 iloveagent57 left a comment

Choose a reason for hiding this comment

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

This is looking good, couple small suggestions for simplicity and performance improvements.

enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/tasks.py Outdated Show resolved Hide resolved
enterprise/tasks.py Outdated Show resolved Hide resolved
@katrinan029 katrinan029 requested a review from iloveagent57 April 5, 2024 17:38
enterprise/api/v1/views/enterprise_group.py Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
# ecus: enterprise customer users
ecus = []
# Gather all existing User objects associated with the email batch
existing_users = User.objects.filter(email__in=user_email_batch)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a red flag to me based on all the problems Alex has with filtering and querying the User table and it taking so long. I would look into this PR and see what he's doing with the SQL string https://github.com/openedx/edx-enterprise/pull/2057/files#diff-d950991245f878f58adc157895d6431af4385d522ab994aa25726e9abc2b434dR4274

Copy link
Contributor

Choose a reason for hiding this comment

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

Though he was doing email__icontains and not email__in which would be faster but maybe it should be something more like the _get_filtered_ecu_ids

Copy link
Contributor

Choose a reason for hiding this comment

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

so I did a little poking around - grabbed the sql query from the queryset and ran it against prod

mysql> SELECT auth_user.id,
    ->        auth_user.password,
    ->        auth_user.last_login,
    ->        auth_user.is_superuser,
    ->        auth_user.username,
    ->        auth_user.first_name,
    ->        auth_user.last_name,
    ->        auth_user.email,
    ->        auth_user.is_staff,
    ->        auth_user.is_active,
    ->        auth_user.date_joined
    -> FROM   auth_user
    -> WHERE  auth_user.email IN ( "[email protected]", "[email protected]" );
+----------+--------------------------------------------------------------------------------+----------------------------+--------------+----------+------------+-----------+-------------------+----------+-----------+---------------------+
| id       | password                                                                       | last_login                 | is_superuser | username | first_name | last_name | email             | is_staff | is_active | date_joined         |
+----------+--------------------------------------------------------------------------------+----------------------------+--------------+----------+------------+-----------+-------------------+----------+-----------+---------------------+
| 40294834 | pbkdf2_sha256$150000$x6piOQTpY9TT$bgr29MbJEahwaqOEXfAoqpFPYndM4v29SSoQYZCVYPU= | 2021-03-11 06:05:35.793195 |            0 | ---2     |            |           | [email protected] |        0 |         0 | 2021-03-11 06:05:35 |
+----------+--------------------------------------------------------------------------------+----------------------------+--------------+----------+------------+-----------+-------------------+----------+-----------+---------------------+
1 row in set (0.00 sec)

I thiiink this shows it's running in a reasonable time but we should def test once it goes out

enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Show resolved Hide resolved
tests/test_enterprise/test_tasks.py Show resolved Hide resolved
Copy link
Contributor

@alex-sheehan-edx alex-sheehan-edx left a comment

Choose a reason for hiding this comment

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

Some thoughts - let me know if you want to discuss

enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Show resolved Hide resolved
# ecus: enterprise customer users
ecus = []
# Gather all existing User objects associated with the email batch
existing_users = User.objects.filter(email__in=user_email_batch)
Copy link
Contributor

Choose a reason for hiding this comment

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

so I did a little poking around - grabbed the sql query from the queryset and ran it against prod

mysql> SELECT auth_user.id,
    ->        auth_user.password,
    ->        auth_user.last_login,
    ->        auth_user.is_superuser,
    ->        auth_user.username,
    ->        auth_user.first_name,
    ->        auth_user.last_name,
    ->        auth_user.email,
    ->        auth_user.is_staff,
    ->        auth_user.is_active,
    ->        auth_user.date_joined
    -> FROM   auth_user
    -> WHERE  auth_user.email IN ( "[email protected]", "[email protected]" );
+----------+--------------------------------------------------------------------------------+----------------------------+--------------+----------+------------+-----------+-------------------+----------+-----------+---------------------+
| id       | password                                                                       | last_login                 | is_superuser | username | first_name | last_name | email             | is_staff | is_active | date_joined         |
+----------+--------------------------------------------------------------------------------+----------------------------+--------------+----------+------------+-----------+-------------------+----------+-----------+---------------------+
| 40294834 | pbkdf2_sha256$150000$x6piOQTpY9TT$bgr29MbJEahwaqOEXfAoqpFPYndM4v29SSoQYZCVYPU= | 2021-03-11 06:05:35.793195 |            0 | ---2     |            |           | [email protected] |        0 |         0 | 2021-03-11 06:05:35 |
+----------+--------------------------------------------------------------------------------+----------------------------+--------------+----------+------------+-----------+-------------------+----------+-----------+---------------------+
1 row in set (0.00 sec)

I thiiink this shows it's running in a reasonable time but we should def test once it goes out

enterprise/api/v1/views/enterprise_group.py Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Show resolved Hide resolved
enterprise/models.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alex-sheehan-edx alex-sheehan-edx left a comment

Choose a reason for hiding this comment

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

Just a few small nits/changes but otherwise looks good to me!

enterprise/models.py Outdated Show resolved Hide resolved
enterprise/api/v1/views/enterprise_group.py Outdated Show resolved Hide resolved
@katrinan029 katrinan029 merged commit 95d4012 into master Apr 18, 2024
8 checks passed
@katrinan029 katrinan029 deleted the knguyen2/ent-8504 branch April 18, 2024 18:20
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.

4 participants