Conversation
This PR adds Ed-Fi parents to the OneRoster users.
| edu_wh.row_pluck(ref('stg_ef3__parents__emails'), | ||
| key='k_parent', | ||
| column='email_type', | ||
| preferred=var('oneroster:student_email_type', 'Home/Personal'), |
There was a problem hiding this comment.
I'm re-using 'oneroster:student_email_type' here; should it be broken out to a separate 'oneroster:parent_email_type'?
| ), | ||
| parent_email as ( | ||
| {{ | ||
| edu_wh.row_pluck(ref('stg_ef3__parents__emails'), |
There was a problem hiding this comment.
Here and on telephones below, I mimicked querying the stage layer like int_users_student does - please see my PR comment about also supporting stg__contacts__emails.
There was a problem hiding this comment.
stg__contacts__emails/telephones should be used instead of stg___parents. the contacts versions have already combined across parents/contacts, and that's what's done in dim_parent with addresses
| group by all | ||
| ), | ||
|
|
||
| parent_students as ( |
There was a problem hiding this comment.
OneRoster has users.agentSourcedIds about which the docs say
Humans may have relationships with other humans. For example, a student may have parents. The "agents" attribute allows for relationships between humans to be expressed. Note that these are typically from the point of view of the student - so a student will link to its parents (via the agent attribute). The reverse view MUST also be modeled, so for example, a user of role "parent" MUST have agents that are of type "student".
This CTE constructs the array of sourcedIds of students for each parent; a similar CTE was added to int_user_student to construct the array of sourcedIds of parents for each student.
| parent_email.email_address as "email", | ||
| null::string as "sms", | ||
| parent_telephone.phone_number as "phone", | ||
| parent_students_agg.students as "agentSourcedIds", |
There was a problem hiding this comment.
I kept agentSourcedIds here because that's what the staff and student users use, but I think according to the OneRoster 1.1 spec it should be just agents?
| select * from {{ ref('fct_student_school_association') }} | ||
| where school_year = {{ var('oneroster:active_school_year')}} | ||
| ), | ||
| {% if var('oneroster:include_parents', False) %} |
There was a problem hiding this comment.
All parent-related logic in the students model is behind the include_parents switch so the model shouldn't slow down if parents aren't included.
There was a problem hiding this comment.
do we think it's more common that implementations want this OFF vs. ON?
There was a problem hiding this comment.
Good question; I don't know.
Are you thinking if the default/common is ON, we should remove these if sections to make the code simpler/more readable?
rlittle08
left a comment
rlittle08
left a comment
There was a problem hiding this comment.
Code looks good & nicely consistent with staff/student. A couple requests
- Can we move the student-parent agent relationship stuff to a bld model for DRYness
- I'm torn on whether we use 'parent' vs. 'contact' naming throughout. Ed-Fi has moved to 'Contact', and EDU should do the same someday. If we go with contact now in this package, then any downstream oneroster apps would be safe from breaking change later on..
|
@rlittle08 thanks for the thorough review.
Sounds good, done. (I tested the latest version of this branch in TX - see the result in
Do you mean things like naming the model |
|
The idea is eventually to rename |
|
@rlittle08 awesome, thanks. I don't have permission to merge or release. What's the process for that - can you just do it? or do we need sign-off from someone else? (This is needed in TX - not extremely urgent but the sooner the better. Do you think it can be released this week?) |
2 participants
This PR adds Ed-Fi parents to the OneRoster users, according to the OneRoster 1.1 specification.
Most parent data here is coming from
dim_parent– which already collapses Ed-Fiparentsandcontacts. However, emails and telephones are coming from (the stage layer) ofparentsonly. I'm curious for suggestions of a more robust approach... or shouldedu_edfi_sourcealso be merging contacts.emails withparents.emails (here), and contacts.telephones with parents.telephones (here)?Separately, OneRoster has not only role
parentbut alsoguardian; in Ed-Fi, guardians and parents both go inparentsorcontacts, with differentstudent[Parent|Contact]Associations.relationshipTypeDescriptors. It's possible to set the OneRoster user'sroledynamically, but would we need a xwalk to map differentrelationshipTypeDescriptorvalues to different OneRosterroles?I tested this in TX, it added 1.8M OneRoster
userswithrole='parent'. Due to the additional CTEs and joins, it runs a little slower (~27s) but this seems reasonable.I'll add a self-review with a few other specific comments/questions.