-
Notifications
You must be signed in to change notification settings - Fork 63
ENT-11235: Updating schema to add invited_date and joined_date fields to customer admin. #2491
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
base: master
Are you sure you want to change the base?
Conversation
|
This is the ticket : |
kiram15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused as to why we are changing fields in the EnterpriseCustomerUser table. When you are going to pull all the admins for the new admin DataTable, we should not be going through a list of the EnterpriseCustomerUsers, but rather another table of all the admins. As I said in the comment, I think the best approach would be adding the invited_date and joined_date fields to EnterpriseCustomerAdmin table, and then plan to pull in the (relevant, non-onboarding tour) information for the DataTable y'all will be creating.
I also think that whenever we're adding new fields, its good to include in the PR how they will be getting populated. Here is how EnterpriseCustomerAdmins are currently created (in a post_save django signal where an "enterprise_admin" system-wide role is assigned to the user). I'm assuming the plan is to have the invited_date be when the PendingEnterpriseCustomerAdminUser record is created, and then a joined_date be populated if/when the EnterpriseCustomerAdmin record is created, but in order for me to review these fields, I need to know more information about these dates.
Don't want to block this PR before I go on vacation
enterprise/migrations/0243_enterprisecustomeradmin_invited_date_and_more.py
Outdated
Show resolved
Hide resolved
|
Before merging, please squash your commits to only one commit and use a more descriptive title, such as "feat: add date fields to track enterprise user invitation lifecycle". Please also change the title of this PR to match that more descriptive title. |
0cda236 to
dc95ac2
Compare
| self._assert_pending_ecus_exist(should_exist=False) | ||
|
|
||
| @mark.django_db | ||
| class TestEnterpriseCustomerAdminSignals(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please verify that invited_date and joined_date fields are set to the correct values, you can extend your test cases to compare against expected timestamps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @msaha-sonata ,
Added the test cases.
msaha-sonata
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
kiram15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you update the changelog and init file, and rename the title and commits like Troy mentioned. Once it's in a good state, I can merge it in tomorrow!
CHANGELOG.rst
Outdated
| --------------------- | ||
| * feat: added atlas translations flow in enterprise app | ||
|
|
||
| [6.5.8] - 2025-12-16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog needs to be updated because of updates that happened since, and also the init file needs to be updated too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kiram15
Done the changes. I have updated the files. could you please do the further process.
339bbdb to
63a0c5f
Compare
pwnage101
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enterprise/models.py
Outdated
| """Track the date and time when a pending enterprise admin invite was created.""" | ||
| invited_date = models.DateTimeField( | ||
| null=True, | ||
| blank=True, | ||
| help_text="Timestamp when the admin invite was created." | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field seems redundant; there's already a created field inherited from TimeStampedModel. Furthermore, this PR lacks code changes for populating this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pwnage101
I am removing invited_date and joined_date, populated invited_date directly at creation time in init.py.
Is that fine?
enterprise/models.py
Outdated
| default=False, | ||
| help_text=_("Whether the admin has completed the onboarding tour.") | ||
| ) | ||
| """Track when the admin was invited.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the incorrect syntax for code comments. Use # prefix:
| """Track when the admin was invited.""" | |
| # Track when the admin was invited. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pwnage101
Done
| help_text="Timestamp when the admin was invited." | ||
| ) | ||
| """Track when the admin accepted the invite and joined.""" | ||
| joined_date = models.DateTimeField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This model also inherits TimeStampedModel, so joined_date is redundant with created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
enterprise/signals.py
Outdated
| instance.display_name = f'SSO-config-{instance.identity_provider}-{num_records_for_customer + 1}' | ||
|
|
||
| @receiver(post_save, sender=EnterpriseCustomerAdmin) | ||
| def populate_admin_onboarding_metadata(sender, instance, created, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this entire signal is not necessary. We have direct access to the code that creates the EnterpriseCustomerAdmins:
edx-enterprise/enterprise/api/__init__.py
Lines 27 to 29 in b42d184
| EnterpriseCustomerAdmin.objects.get_or_create( | |
| enterprise_customer_user=enterprise_customer_user, | |
| ) |
You only need to update that call to look like this:
EnterpriseCustomerAdmin.objects.get_or_create(
enterprise_customer_user=enterprise_customer_user,
defaults={'invited_date': pending_admin_user.created},
)We typically reserve signals in cases where direct code manipulation is not possible, or breaks modularity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pwnage101
I have worked on this .
could you please review the same and let me any changes required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
e64e451 to
dce1bd4
Compare
| enterprise_customer_user=eu, | ||
| last_login=timezone.now() | ||
| last_login=timezone.now(), | ||
| joined_date=timezone.now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: leave a trailing comma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -0,0 +1,49 @@ | |||
| # Generated by Django 5.2.8 on 2025-12-29 17:47 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you forgot to delete this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pwnage101
Deleted the file.
enterprise/models.py
Outdated
| default=False, | ||
| help_text=_("Whether the admin has completed the onboarding tour.") | ||
| ) | ||
| """Track when the admin accepted the invite and joined.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the incorrect syntax for code comments. Use # prefix:
| """Track when the admin accepted the invite and joined.""" | |
| # Track when the admin accepted the invite and joined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| ).count() | ||
| instance.display_name = f'SSO-config-{instance.identity_provider}-{num_records_for_customer + 1}' | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-add empty line to simplify diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added empty line.
enterprise/signals.py
Outdated
| from django.utils.timezone import now | ||
|
|
||
| from enterprise import models, roles_api | ||
| from enterprise.api import activate_admin_permissions | ||
| from enterprise.api_client.enterprise_catalog import EnterpriseCatalogApiClient | ||
| from enterprise.decorators import disable_for_loaddata | ||
| from enterprise.models import ( | ||
| EnterpriseCustomerAdmin, | ||
| PendingEnterpriseCustomerAdminUser, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove added imports which are no longer used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| [6.6.2] - 2026-01-15 | ||
| --------------------- | ||
| * feat: add a waffle flag for invite admins | ||
|
|
||
| [6.6.1] - 2026-01-09 | ||
| --------------------- | ||
| * fix: issue regarding the gettext package in atlas translations flow | ||
|
|
||
| [6.6.0] - 2026-01-08 | ||
| --------------------- | ||
| * feat: added atlas translations flow in enterprise app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your commit should never include changelog entries from other commits. You still need to rebase your commit onto the upstream origin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
enterprise/__init__.py
Outdated
| Your project description goes here. | ||
| """ | ||
|
|
||
| __version__ = "6.6.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the version jump is this large, you need to rebase your commit on the origin (openedx/edx-enterprise master branch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| self._assert_pending_ecus_exist(should_exist=False) | ||
|
|
||
| @mark.django_db | ||
| class TestEnterpriseCustomerAdminSignals(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature no longer uses signals, so it should not be considered a signals test. You should relocate these test cases. Probably tests/test_models.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE.
be1db8e to
bf159c2
Compare
…and add invitation lifecycle fields removed signal and set invited_date via defaults updating changelog.rst Removed unintended migration file fixed comments
bf159c2 to
bd4b650
Compare
|
Hi @pwnage101 , |

Merge checklist:
requirements/*.txtfiles)base.inif needed in production but edx-platform doesn't install ittest-master.inif edx-platform pins it, with a matching versionmake upgrade && make requirementshave been run to regenerate requirementsmake statichas been run to update webpack bundling if any static content was updated./manage.py makemigrationshas been run./manage.py lms makemigrationsin the shell.Post merge:
(so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
make upgradein edx-platform will look for the latest version in PyPi.