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

Allow import of tardy and absence events in single record #2658

Merged

Conversation

edavidsonsawyer
Copy link
Collaborator

@edavidsonsawyer edavidsonsawyer commented Oct 2, 2019

This addresses #1925 by creating both an absence and a tardy if both exist in the record.

@@ -189,7 +199,7 @@ def attendance_event_class(row)
is_absence = per_district.is_attendance_import_value_truthy?(row[:absence])
is_tardy = per_district.is_attendance_import_value_truthy?(row[:tardy])

if is_absence && is_tardy then nil
if is_absence && is_tardy then "both"
Copy link
Contributor

@kevinrobinson kevinrobinson Oct 2, 2019

Choose a reason for hiding this comment

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

This seems like a good point to make the change, but using a string here means that attendance_event_class sometimes returns a class and sometimes returns a special string value. How about renaming the method to reflect that change and use a symbol to indicate it's a special value? Or alternately, you could have it return a list of classes (eg,[Absence, Tardy]).

Copy link
Collaborator Author

@edavidsonsawyer edavidsonsawyer Oct 3, 2019

Choose a reason for hiding this comment

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

Makes sense. How about we rename attendance_event_class and it just returns an array of valid classes for the event. That way we're always dealing with the same kind of thing.

One question I've got. It's fair to assume the detailed fields should be the same if the row contains both an absence and a tardy?

Copy link
Contributor

@kevinrobinson kevinrobinson Oct 3, 2019

Choose a reason for hiding this comment

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

Sure, that sounds good 👍

re: the detailed fields, the shape of the export file is the same (eg, these tests don't look like they use a fixture CSV, but the test setup reflects this too), and the shape of the Insights models are the same (eg, check out schema.rb). Whether the meaning of these fields beyond their types are the same (eg, what do the letter codes within these fields mean), or whether data entry and quality is the same (eg, does "excused" mean the same thing across places within the district) is more complicated :)

But I think for this PR we can ship it and it'll make tardy counts more accurate, and then we can look at the deeper level within Insights records if those kinds of questions come up down the line.

@kevinrobinson
Copy link
Contributor

@kevinrobinson kevinrobinson commented Oct 3, 2019

🚢 !

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.

None yet

2 participants