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

feature/person exodus #268

Merged
merged 48 commits into from
Jan 11, 2021
Merged

feature/person exodus #268

merged 48 commits into from
Jan 11, 2021

Conversation

thatbudakguy
Copy link
Contributor

@thatbudakguy thatbudakguy commented Jan 6, 2021

This PR addresses #186 and #253, but not #267 (the person list pages). It's a big one!

Some things to note:

  • Multiple migrations that un-proxy the old Person model and make it independent from django's User
  • Most properties of Profile move to the new Person (office location, phone, job title, etc.)
  • Profile becomes a new wagtail Page model called ProfilePage (but the old Profile still exists, so it can be exodized)
  • The admin section for this app is redone so that ProfilePages are managed through a special menu (similar to the current admin) and don't clutter the wagtail page tree - check out wagtail_hooks.py
  • Some fancy Wagtail admin features are added, such as automatic export of people to excel/csv and inline editing of Titles on people
  • A special PeopleLandingPage is created so that correct page hierarchy can be enforced through code
  • I adopted a new convention (criticism welcome) of moving model-related tests for wagtail Page models to a file called test_pages.py for each app, so that test_models.py was less crowded and could focus on non-Page models.
  • I tried to reduce this app's reliance on a single large fixture, which led to rewriting many tests and a lot of repetition of set-up code. This could be refactored later.

rlskoeser and others added 30 commits December 22, 2020 11:39
@codecov-io
Copy link

codecov-io commented Jan 6, 2021

Codecov Report

Merging #268 (771a70d) into develop (8d7c85b) will decrease coverage by 10.03%.
The diff coverage is 95.49%.

@@             Coverage Diff              @@
##           develop     #268       +/-   ##
============================================
- Coverage    97.68%   87.64%   -10.04%     
============================================
  Files           99      125       +26     
  Lines         3150     3124       -26     
============================================
- Hits          3077     2738      -339     
- Misses          73      386      +313     

Copy link
Contributor

@rlskoeser rlskoeser left a comment

Choose a reason for hiding this comment

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

Ok. I think I have looked at all the files (what a monster of a PR). Haven't run anything locally yet, there may be a few more things I need to look at when I do.

Happy with your decision to split out unit tests more, they needed to be broken out more.

cdhweb/pages/wagtail_hooks.py Show resolved Hide resolved
cdhweb/people/migrations/0011_populate_nonproxy_person.py Outdated Show resolved Hide resolved
cdhweb/people/migrations/0011_populate_nonproxy_person.py Outdated Show resolved Hide resolved
cdhweb/people/migrations/0011_populate_nonproxy_person.py Outdated Show resolved Hide resolved
cdhweb/pages/management/commands/exodus.py Show resolved Hide resolved
cdhweb/pages/management/commands/exodus.py Outdated Show resolved Hide resolved
cdhweb/pages/management/commands/exodus.py Show resolved Hide resolved
cdhweb/pages/management/commands/exodus.py Show resolved Hide resolved
cdhweb/pages/models.py Show resolved Hide resolved
Copy link
Contributor

@rlskoeser rlskoeser left a comment

Choose a reason for hiding this comment

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

Updates since my last review look good; just one comment on some lines that I think are inside a for loop and shouldn't be.

cdhweb/people/migrations/0011_populate_nonproxy_person.py Outdated Show resolved Hide resolved
@thatbudakguy thatbudakguy merged commit 0aa2ae9 into develop Jan 11, 2021
@thatbudakguy thatbudakguy deleted the feature/person-exodus branch January 11, 2021 19:11
@thatbudakguy thatbudakguy mentioned this pull request Feb 2, 2021
2 tasks
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