-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
Co-authored-by: Nick Budak <[email protected]>
Co-authored-by: Nick Budak <[email protected]>
Co-authored-by: Nick Budak <[email protected]>
Co-authored-by: Rebecca Sutton Koeser <[email protected]>
Co-authored-by: Nick Budak <[email protected]>
Co-authored-by: Nick Budak <[email protected]>
Codecov Report
@@ 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 |
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.
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.
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.
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.
This PR addresses #186 and #253, but not #267 (the person list pages). It's a big one!
Some things to note:
Person
model and make it independent from django'sUser
Profile
move to the newPerson
(office location, phone, job title, etc.)Profile
becomes a new wagtailPage
model calledProfilePage
(but the oldProfile
still exists, so it can be exodized)ProfilePage
s are managed through a special menu (similar to the current admin) and don't clutter the wagtail page tree - check outwagtail_hooks.py
Title
s on peoplePeopleLandingPage
is created so that correct page hierarchy can be enforced through codePage
models to a file calledtest_pages.py
for each app, so thattest_models.py
was less crowded and could focus on non-Page
models.