ENT-11235: update schema #2491
ENT-11235: update schema #2491
Conversation
|
This is the ticket : |
kiram15
left a comment
kiram15
left a comment
There was a problem hiding this comment.
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.
kiram15
dismissed
their stale review
Don't want to block this PR before I go on vacation
| model_name="enterprisecustomeradmin", | ||
| name="invited_date", | ||
| field=models.DateTimeField( | ||
| blank=True, help_text="Timestamp when the admin was invited.", null=True |
There was a problem hiding this comment.
nit: have these all on different lines for readability
| blank=True, | ||
| help_text="Timestamp when the admin was invited." | ||
| ) | ||
|
|
enterprise/api/v1/serializers.py
Outdated
|
|
||
| def is_enterprise_customer_user(self, obj): | ||
| return hasattr(obj, 'user_id') and obj.user_id > 0 | ||
| return bool(obj.user_id) |
There was a problem hiding this comment.
Why are we changing this? And I don't see it getting used anywhere?
| ).count() | ||
| instance.display_name = f'SSO-config-{instance.identity_provider}-{num_records_for_customer + 1}' | ||
|
|
||
| @receiver(post_save, sender=EnterpriseCustomerAdmin) |
There was a problem hiding this comment.
We should also have a test to make sure this signal is working correctly and validating the invited and joined dates, here is the test_signals file where it would be.
| # Attempt to backfill invitation metadata | ||
| pending_invite = PendingEnterpriseCustomerAdminUser.objects.filter( | ||
| enterprise_customer=instance.enterprise_customer_user.enterprise_customer, | ||
| user_email=instance.enterprise_customer_user.user_email, | ||
| ).order_by("-invited_date").first() |
There was a problem hiding this comment.
echoing Alex's question as I have the same concern. Do PendingEnterpriseCustomerAdminUser records get deleted? If so, will this PendingEnterpriseCustomerAdminUser instance exist at execution time of this hook?
There was a problem hiding this comment.
Yes, they are deleted but not before this signal in normal flows.
|
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. |
sjasti-sonata-svg
force-pushed
the
feat/admin-invite-metadata-ENT-11235
branch
from
3e7863b to
38bf777
Compare
…and add invitation lifecycle fields - Fixed the invited_date signal handling for EnterpriseCustomerAdmin - Updated related models and signals - Added date fields to track enterprise user invitation lifecycle
sjasti-sonata-svg
force-pushed
the
feat/admin-invite-metadata-ENT-11235
branch
from
0cda236 to
dc95ac2
Compare
3 participants
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.