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: replace references to User with references to Person #6024

Merged
merged 22 commits into from
Jan 24, 2024

Conversation

rjsparks
Copy link
Member

No description provided.

pselkirk and others added 12 commits June 20, 2023 12:51
* refactor: Change CommunityList reference from User to Person

* refactor: Convert more user references to person

* refactor: Change augment_docs_and_user_with_user_info to person

* refactor: Change Nomination and Feedback references from User to Person

* refactor: Change a few test case function signatures to be more pythonic
…he URL

This only happens using the form-filling and submission feature of
WebTest, which is only used in this one test case, so just it rip out.
refactor: Rework community views to operate on Person instead of User
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (b38e8c3) 88.80% compared to head (bf6ae7d) 88.91%.
Report is 9 commits behind head on main.

Files Patch % Lines
ietf/community/views.py 80.00% 12 Missing ⚠️
ietf/doc/utils.py 88.88% 1 Missing ⚠️
ietf/nomcom/utils.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6024      +/-   ##
==========================================
+ Coverage   88.80%   88.91%   +0.11%     
==========================================
  Files         285      289       +4     
  Lines       40383    40454      +71     
==========================================
+ Hits        35862    35971     +109     
+ Misses       4521     4483      -38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rjsparks
Copy link
Member Author

rjsparks commented Aug 11, 2023

Leaving some notes here from a deep dive into the edge cases.

There are users without persons.

These migrations currently would lose some data about who (for a very poor value of who - all we would lose is a username since there's no person connected already) made nominations in very old nomcoms. That data is supposed to have been removed already anyhow, so I'm not particularly concerned about that.

There are communitylists attacted to users without persons. Unless these also have EmailSubscriptions, they don't do anything anyhow and deleting them is safe. There is only one that has an EmailSubscription attached at the moment. I'll appoach the owner and adjust that one away.

Then we should add explicitly removing the personless objects instead of migrating them to person=None - the migrations will need to be updated.

# Conflicts:
#	ietf/community/tests.py
#	ietf/community/views.py
#	ietf/templates/doc/document_draft.html
#	ietf/templates/doc/search/search_result_row.html
#	requirements.txt
…erson

chore: Merge main into feat/user2person
* fix: Use refactored method

* fix: Don't assume user has person

* fix: Use new view param name
@jennifer-richards
Copy link
Member

There are communitylists attacted to users without persons. Unless these also have EmailSubscriptions, they don't do anything anyhow and deleting them is safe. There is only one that has an EmailSubscription attached at the moment. I'll appoach the owner and adjust that one away.

This appears to have been done. On production:

>>> CommunityList.objects.filter(user__person__isnull=True).exclude(user__isnull=True).exclude(emailsubscription__isnull=True).count()
0

@jennifer-richards
Copy link
Member

These migrations currently would lose some data about who (for a very poor value of who - all we would lose is a username since there's no person connected already) made nominations in very old nomcoms. That data is supposed to have been removed already anyhow, so I'm not particularly concerned about that.

Looks like there are only 5 nominations affected if I'm understanding this correctly. The most recent was in 2018.

datatracker=> SELECT COUNT(*)
datatracker-> FROM nomcom_nomination nom
datatracker->          LEFT OUTER JOIN person_person p USING (user_id)
datatracker-> WHERE nom.user_id IS NOT NULL
datatracker->   AND p.id IS NULL;
 count
-------
     5
(1 row)

There are also 51 nominations with no User at all, the latest in 2019.

@jennifer-richards
Copy link
Member

Looks like there are also 199 Feedback instances tied to Personless users. I'm guessing these are in the same category as the Nomination instances - there's still a field indicating the author, there's just no link to a Person. The latest of these were in 2021.

jennifer-richards and others added 4 commits January 8, 2024 13:15
* fix: Don't assume user has person

* fix: user->person in update_community_list_index.py

* feat: Remove CommunityLists without Person

* refactor: Speed up nomcom migrations
@rjsparks rjsparks merged commit d9cc26b into main Jan 24, 2024
9 checks passed
@rjsparks rjsparks deleted the feat/user2person branch January 25, 2024 21:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants