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

WIP: Clean user records #482

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/peoplefinder/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
from peoplefinder.forms.admin import TeamModelForm
from peoplefinder.models import LegacyAuditLog, NewNetwork, Person, Team, TeamMember
from peoplefinder.services.team import TeamService
from user.models import User


class PersonModelAdmin(admin.ModelAdmin):
"""Admin page for the Person model."""

list_display = ["full_name", "email"]
list_display = ["full_name", "email", "is_active"]
list_filter = ["is_active"]
search_fields = [
"first_name",
Expand All @@ -21,6 +22,9 @@ class PersonModelAdmin(admin.ModelAdmin):
@admin.action(description="Mark selected people as active")
def make_active(self, request, queryset):
queryset.filter(is_active=False).update(is_active=True, became_inactive=None)
User.objects.filter(profile__pk__in=queryset.values("pk")).update(
is_active=True
)


class LegacyAuditLogModelAdmin(admin.ModelAdmin):
Expand Down
4 changes: 4 additions & 0 deletions src/peoplefinder/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,10 @@ def save(self, *args, **kwargs):
from peoplefinder.services.person import PersonService

self.profile_completion = PersonService().get_profile_completion(person=self)

if self.user and self.is_active != self.user.is_active:
self.user.is_active = self.is_active
Copy link
Contributor

Choose a reason for hiding this comment

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

If the profile is set to active and the user is set to inactive, it will update the user to be active, is this the desired behaviour?

Is there any way that updates the User is active flag? (maybe from SSO?)

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 put this as provocation - my assumption is whichever record (User / Person) is being saved, you probably want the other record's is_active to match.... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the sso connection is waiting on a erlated PR but is there in the management command; I could abstract this to a service

self.user.save()
return super().save(*args, **kwargs)

def get_first_name_display(self) -> str:
Expand Down
3 changes: 3 additions & 0 deletions src/peoplefinder/services/person.py
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,9 @@ def profile_deleted(
person.is_active = False
person.became_inactive = timezone.now()
person.save()
if person.user:
person.user.is_active = False
person.user.save()

AuditLogService().log(AuditLog.Action.DELETE, deleted_by, person)

Expand Down
3 changes: 3 additions & 0 deletions src/peoplefinder/views/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,9 @@ def action(self):
self.person.is_active = True
self.person.became_inactive = None
self.person.save()
if self.person.user:
self.person.user.is_active = True
self.person.user.save()


class ProfileUpdateUserView(SuccessMessageMixin, HtmxFormView):
Expand Down
101 changes: 101 additions & 0 deletions src/user/management/commands/archive_users.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
from datetime import timedelta
import requests
from urllib.parse import urlparse, urlunparse

from django.conf import settings
from django.core.management.base import BaseCommand
from django.utils import timezone

from user.models import User
from peoplefinder.models import Person


class Command(BaseCommand):
help = "Clean up and archive User and Person records"

def add_arguments(self, parser):
parser.add_argument("days", type=int, default=90)
parser.add_argument("limit", type=int, default=25)
parser.add_argument("offset", type=int, default=0)

def handle(self, *args, **options):
days = options["days"]
limit = options["limit"]
offset = options["offset"]

self.sync_inactive_users_profiles()

login_cutoff_date = timezone.now() - timedelta(days=days)
users_to_check = User.objects.filter(is_active=True).filter(
last_login__lt=login_cutoff_date
)
number_of_users = users_to_check.count()

if limit == 0:
batch = users_to_check
self.stdout.write(
self.style.NOTICE(
f"Checking all {number_of_users} User records with last_login older than {days} ago"
)
)
else:
batch = users_to_check[offset:limit]
self.stdout.write(
self.style.NOTICE(
f"Checking {limit} (from record {offset}) of {number_of_users} User records with last_login older than {days} ago"
)
)

deactivated = 0
ignored = 0
# check against SSO API endpoint to see if user is active there
authbroker_url = urlparse(settings.AUTHBROKER_URL)
url = urlunparse(authbroker_url._replace(path="/introspect/"))
headers = {"Authorization": f"bearer {settings.AUTHBROKER_INTROSPECTION_TOKEN}"}

for user in batch:
params = {"email": user.email}
response = requests.get(url, params, headers=headers, timeout=5)
if response.status_code == 200:
resp_json = response.json()
if not resp_json["is_active"]:
user.is_active = False
user.save()
deactivated += 1
else:
ignored += 1
else:
self.stdout.write(
self.style.ERROR(
f"SSO introspect endpoint returned {response.status_code} status code for {user.email}"
)
)

self.stdout.write(
self.style.SUCCESS(
"Job finished successfully\n"
f"{deactivated} User records marked as inactive\n"
f"{ignored} User records left as active\n"
)
)

def sync_inactive_users_profiles(self):
mismatched_users = User.objects.filter(profile__is_active=False).filter(
is_active=True
)
mismatched_users.update(active=False)
self.stdout.write(
self.style.SUCCESS(
f"Updated {mismatched_users.count()} active User records with inactive Person profiles to inactive"
)
)

mismatched_profiles = Person.objects.filter(is_active=True).filter(
user__is_active=False
)
mismatched_profiles.update(active=False)
self.stdout.write(
self.style.SUCCESS(
f"Updated {mismatched_profiles.count()} active Person records with inactive User records to inactive"
)
)
61 changes: 61 additions & 0 deletions src/user/management/commands/clean_user_records.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
from django.core.management.base import BaseCommand
from django.db.models import Count

from user.models import User
from peoplefinder.models import Person


class Command(BaseCommand):
help = "Clean up User and Person records"

def handle(self, *args, **options):
users_without_profile = User.objects.filter(profile__isnull=True)
self.do_delete(users_without_profile, "User")

profiles_without_user = Person.objects.filter(user__isnull=True)
self.do_delete(profiles_without_user, "Person")

no_duplicate_emails = (
User.objects.values("email")
.annotate(email_count=Count("email"))
.filter(email_count__gt=1)
.count()
)
if no_duplicate_emails > 0:
self.stdout.write(
self.style.NOTICE(
f"Found {no_duplicate_emails} User records with duplicate emails"
)
)

def do_delete(self, queryset, model_name):
self.stdout.write(
self.style.NOTICE(
f"Found {queryset.count()} {model_name} records with no associated Person profile"
)
)
try:
queryset.delete()
self.stdout.write(
self.style.SUCCESS("Batch {model_name} deletion successful")
)
except Exception as e:
self.stderr.write(
self.style.ERROR(f"Batch {model_name} deletion stopped with error: {e}")
)
count = 0
for record in queryset:
try:
record.delete()
count += 1
except Exception as e:
self.stderr.write(
self.style.ERROR(
f"{model_name} {record} deletion raised error: {e}"
)
)
self.stdout.write(
self.style.SUCCESS(
"One-by-one deletion successful for {count} {model_name} records"
)
)
6 changes: 6 additions & 0 deletions src/user/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,11 @@ class Meta:
def __str__(self):
return f"{self.first_name} {self.last_name}"

def save(self, *args, **kwargs):
if hasattr(self, "profile") and self.is_active != self.profile.is_active:
self.profile.is_active = self.is_active
self.profile.save()
return super().save(*args, **kwargs)


register(User)