Skip to content
This repository has been archived by the owner on Jun 18, 2024. It is now read-only.

[872/827] Make staff role fields dependent on staff type #115

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tm-kn
Copy link
Member

@tm-kn tm-kn commented Jul 6, 2017

No description provided.

@tm-kn tm-kn self-assigned this Jul 6, 2017
@tm-kn tm-kn requested a review from balazs-endresz July 6, 2017 22:00
@tm-kn tm-kn force-pushed the 827-staff-role-dependency-on-staff-type branch from 31d4446 to ce5e0fd Compare July 6, 2017 22:06
for staff in records:
first_role = staff.roles.filter(area__isnull=True).first()
if first_role is not None:
first_role.area = staff.area
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to copy the location field too from the StaffPage? Maybe only if it has one role only?

first_role = staff.roles.filter(area__isnull=True).first()
if first_role is not None:
first_role.area = staff.area
first_role.save()
Copy link
Member

Choose a reason for hiding this comment

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

This won't call clean() automatically, so you might want to do that and provide a list of pages that fail the new validation. You could just do that manually too, not necessarily in the migration.

Copy link
Member Author

@tm-kn tm-kn Aug 3, 2017

Choose a reason for hiding this comment

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

In the newest diff I moved the validation to forms because it would not validate related models properly. I don't think we need to call clean(). Some of the data can stay even if it's not supposed to be there. It will just add unnecessary complexity and if page will be saved later on in the admin site, it'll delete those entries anyway and it will be invisible process.

I also improved migrations to copy location and update draft versions of the pages too.

There's a lot of page that seem that they need upgrading and it's easy enough to make it as migration IMO.

@tm-kn tm-kn force-pushed the 827-staff-role-dependency-on-staff-type branch 2 times, most recently from 1bc961a to c6dd3e4 Compare August 3, 2017 19:38
@tm-kn tm-kn force-pushed the 827-staff-role-dependency-on-staff-type branch from c6dd3e4 to 9ee0cd4 Compare August 4, 2017 13:15
@tm-kn tm-kn changed the title [827] Make staff role fields dependent on staff type [872/827] Make staff role fields dependent on staff type Aug 9, 2017
@tm-kn
Copy link
Member Author

tm-kn commented Aug 10, 2017

Please merge only if #121 is merged. Needs adding dependency of ('taxonomy', '0022_move_area_content_types'), to the 0088_staffpage_migrate_area_data_to_roles (number may change) migration.

Otherwise migrations won't run in order.

Please see fix on staging 554dae0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants