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

Fix bug where school doesn't show up in PD auto emails. #16794

Merged
merged 6 commits into from Aug 7, 2017

Conversation

aoby
Copy link
Contributor

@aoby aoby commented Aug 2, 2017

Add helper methods and tests.

Bug

Now, the mail preview looks like this:
image

@aoby
Copy link
Contributor Author

aoby commented Aug 4, 2017

In the process I cleaned up some of the enrollment validations, moving toward allowing old enrollments with old rules to be grandfathered in and validating new ones more strictly.

I also added a default school_info factory.

@@ -6,7 +6,6 @@ class Api::V1::Pd::WorkshopEnrollmentsControllerTest < ::ActionController::TestC
@facilitator = create :facilitator

@workshop = create :pd_workshop, organizer: @organizer, facilitators: [@facilitator]
@school_info = create :school_info_without_country
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was unused

@@ -222,6 +221,19 @@ def self.get_safe_names
all.map {|enrollment| [enrollment.full_name, enrollment]}
end

# TODO: Delete school column
def school
ActiveSupport::Deprecation.warn('School is deprecated. Use school_info or school_name instead.')
Copy link
Contributor

Choose a reason for hiding this comment

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

You should still return school here for now

@@ -222,6 +221,19 @@ def self.get_safe_names
all.map {|enrollment| [enrollment.full_name, enrollment]}
end

# TODO: Delete school column
Copy link
Contributor

Choose a reason for hiding this comment

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

We just need to make sure that getting rid of the column waits on making use of the existing school data (which is on my plate soon)

Copy link
Contributor

@ewjordan ewjordan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding good test coverage for the changes!

@aoby aoby merged commit 192d3f8 into staging Aug 7, 2017
@aoby aoby deleted the pd-enrollment-school branch August 7, 2017 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants