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

Add regional partner workshops lookup APIs needed by the teacher application #19356

Merged
merged 5 commits into from Nov 22, 2017

Conversation

aoby
Copy link
Contributor

@aoby aoby commented Nov 21, 2017

No description provided.

@aoby aoby changed the base branch from staging to staging-next November 21, 2017 20:56
@aoby
Copy link
Contributor Author

aoby commented Nov 21, 2017

Copy link
Contributor

@mehalshah mehalshah left a comment

Choose a reason for hiding this comment

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

LGTM but one pending question on DC teachers that I'm asking Sarah right now

@@ -15,4 +15,6 @@
class RegionalPartnerProgramManager < ActiveRecord::Base
belongs_to :program_manager, class_name: 'User'
belongs_to :regional_partner

has_many :pd_workshops, class_name: 'Pd::Workshop', through: :program_manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to return the list of workshops a user is attending, organizing, facilitating?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pd::Workshop has a regional_partner, maybe go through that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Organizing, via this association on User. Perhaps, instead of pd_workshops, we should call it something like `pd_workshops_organizing ?

That is not set for newly created workshops. AFAIK we had a one time migration to populate it, and it's otherwise it's a placeholder for a future feature - where the UI will (eventually) give partner organizers the choice of organizing a workshop as themselves or as the partner (which could then be shared with other program managers for the same partner).

@partner_organizer = create :workshop_organizer, :as_regional_partner_program_manager
@non_partner_organizer = create :workshop_organizer

@partner_csd_workshop = create :pd_workshop, organizer: @partner_organizer,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably DRY this

@partner_cds_workshop, @partner_csp_workshop, etc.. = ['csd', 'csp'].map do |course|
  [@partner, @non_partner].map do |partner|
    create  :pd_workshop, organizer: partner, course: course
  end
end.flatten

# @param [String|Symbol] name - full state name, e.g. "Washington"
# @param [Boolean] include_dc - (default: false) Whether to include Washington DC as a state.
# @returns [String] state abbreviation, or nil
def get_us_state_abbr_from_name(name, include_dc = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we ever want to exclude DC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know of, but the inverse method had that option so I added it for consistency

end

def test_get_us_state_abbr_from_name_washington_dc
assert_nil get_us_state_abbr_from_name(:washington_dc)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also DRY this with nested loops - up to you if you want to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stuck with the existing pattern from the rest of this test, and I'd rather not invest more time there now

@@ -315,6 +315,8 @@ def self.should_have_ended
in_state(STATE_IN_PROGRESS).scheduled_end_on_or_before(Time.zone.now - 2.days)
end

scope :organized_by_regional_partners, -> {joins(organizer: :regional_partner_program_managers)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just realized this isn't used. I ended up joining a different way. I'll remove it and the associated test.

Copy link

@clareconstantine clareconstantine left a comment

Choose a reason for hiding this comment

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

Yay tests!

@aoby aoby force-pushed the pd-teacher-application-support-apis branch from 89b854d to 24d0e75 Compare November 22, 2017 01:57
@aoby aoby merged commit a13a635 into staging-next Nov 22, 2017
@aoby aoby deleted the pd-teacher-application-support-apis branch November 22, 2017 16:41
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