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 on_map and funded columns to Pd::Workshop #14660

Merged
merged 2 commits into from Apr 27, 2017
Merged

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Apr 26, 2017

As the first step in the eventual restructuring of the workshop_type tuple to a pair of bools, captured in this trello card.

Based on this mapping:

  Funded Unfunded
On Map Public N/A
Not on Map Private District

Next steps (each in their own PR) will be:

  1. Update the controller model to populate these fields for new workshops based on the value of workshop_type, and release a data migration to back-populate already-created workshops
  2. Update payment calculations (and anything else not client-facing that examines workshop_type) to rely instead on these fields
  3. Remove workshop_type column
  4. (after announcement has been sent and documentation has been updated) Update user-facing UI (workshop creation and edit form) to replace workshop_type dropdown with a pair of radio buttons

@Hamms
Copy link
Contributor Author

Hamms commented Apr 26, 2017

@aoby, @ashercodeorg thoughts on this plan of attack?

class AddPdWorkshopOnMapAndFunded < ActiveRecord::Migration[5.0]
def change
add_column :pd_workshops, :on_map, :boolean
add_column :pd_workshops, :funded, :boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be required (null: false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They will be updated to be so in step 2, as only at that point will we have data for all of em

Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't obvious (at least to me) what these columns mean, even after reading the Trello card and your (otherwise nice) PR summary. Mind adding a brief explanation of these fields in this file? Thanks!

@aoby
Copy link
Contributor

aoby commented Apr 26, 2017

Plan of attack SGTM

Copy link
Contributor

@ashercodeorg ashercodeorg left a comment

Choose a reason for hiding this comment

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

LGTM (as does the plan of attack). Depending on how many rows are touched, I'd recommend doing the data changes in a migration (if not many) or a one-off script (if lots).

class AddPdWorkshopOnMapAndFunded < ActiveRecord::Migration[5.0]
def change
add_column :pd_workshops, :on_map, :boolean
add_column :pd_workshops, :funded, :boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't obvious (at least to me) what these columns mean, even after reading the Trello card and your (otherwise nice) PR summary. Mind adding a brief explanation of these fields in this file? Thanks!

@aoby
Copy link
Contributor

aoby commented Apr 26, 2017

RE: column explanations, you can now add column comments, which are stored in the DB.

@Hamms
Copy link
Contributor Author

Hamms commented Apr 27, 2017

There are ~1500 rows in the workshops table, which to my mind falls into the "not many" category.

Edit: actually, there are ~1800 rows, but that still seems like few enough.

@Hamms Hamms force-pushed the add-on_map-and-funded-cols branch from 0024ce8 to 3148b84 Compare April 27, 2017 19:29
@Hamms Hamms merged commit 4faad64 into staging Apr 27, 2017
@Hamms Hamms deleted the add-on_map-and-funded-cols branch May 9, 2017 20:09
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