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

Feedback properties #299

Merged
merged 8 commits into from
Apr 25, 2017
Merged

Feedback properties #299

merged 8 commits into from
Apr 25, 2017

Conversation

higs4281
Copy link
Member

@higs4281 higs4281 commented Apr 23, 2017

Add Feedback model properties to assist in analysis

This addresses GHE paying-for-college/dynamic-disclosures-dev issue 226

The PR also makes some minor script fixes that address #298:

  • A bug in the update_colleges script that caused grad_rates to fail to update

Additions

  • Four model-method properties for the Feedback model
  • tests for the new methods

This PR also makes some minor fixes:
- A bug in the `update_colleges` script that caused grad_rates to fail to update
- Some documentation updates
@higs4281 higs4281 requested a review from amymok April 23, 2017 03:23
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ee4b098 on feedback-properties into d7c5ecc on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling bad44cb on feedback-properties into 0a2dda8 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b826760 on feedback-properties into 0a2dda8 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4c9292b on feedback-properties into 0a2dda8 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3a427e6 on feedback-properties into 0a2dda8 on master.

Copy link
Member

@willbarton willbarton left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. Nice style cleanup. Lack of familiarity with the code means I'm going over this with a fine-toothed nit picker, but neither of my comments are blockers.

if row and row.get('iped'):
return School.objects.get(pk=row['iped'])
else:
return ''
Copy link
Member

Choose a reason for hiding this comment

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

Is a '' return here (and line 758) appropriate in place of a School? I don't see right off where this is called, so I'm not sure.

def total_fields(field_list):
total = 0
for field in field_list:
if field in url_data.keys() and url_data[field] != '':
Copy link
Member

Choose a reason for hiding this comment

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

if url_data.get('field', '') != '' might be simpler here.

@higs4281
Copy link
Member Author

@willbarton Thanks for the close review. suggestions taken!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f32b520 on feedback-properties into 0a2dda8 on master.

@higs4281 higs4281 merged commit 52b5a4f into master Apr 25, 2017
@higs4281 higs4281 deleted the feedback-properties branch April 25, 2017 16:05
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

3 participants