bulk add opportunities

[chadwhitacre]

Closes #338.

To-do

• add a little inline documentation of the format
• add a form field to select the organizer based on available organizers for logged-in user
• implement idempotency on name
• provide detailed tabular error reporting

[dmtroyer]

http://guides.rubyonrails.org/form_helpers.html

[chadwhitacre]

!m @dmtroyer

[chadwhitacre]

Over to you, @dmtroyer! :-)

[dmtroyer]

[chadwhitacre]

!m @dmtroyer

[dmtroyer]

Currently, the csv parsing logic expects a header in the csv which defines the names of the fields being uploaded in the order listed in the header.

name,description
Marbles,"David's got marbles"
"Skeet shooting","Shoot some skeet"

[chadwhitacre]

Getting back into this, hitting undefined method id' for nil:NilClassinopportunities_controller.rb:39`:

      Opportunity.transaction do
        CSV.parse(params[:csv].read, headers: true) do |row|
          row['resource_sub_type_id'] = ResourceSubType.find_by_name(row.delete('resource_sub_type')).id
          @opportunities.push(Opportunity.create!(row.to_h))
        end
      end

[chadwhitacre]

That's with the test data from above at #341 (comment). Looks like 266ce91 added an expectation for the CSV structure ...

[chadwhitacre]

Blam!

name,description,resource_sub_type
Marbles,"David's got marbles",Event
"Skeet shooting","Shoot some skeet",Event

[chadwhitacre]

(For my notes: I found the database config in config/database.yml, and then found the available resource subtypes like so:

learn_dev=# select * from resource_sub_types;
┌────┬───────┬──────────────────┐
│ id │ name  │ resource_type_id │
├────┼───────┼──────────────────┤
│  1 │ Event │                1 │
└────┴───────┴──────────────────┘
(1 row)

learn_dev=#

)

[chadwhitacre]

[chadwhitacre]

I've started a to-do list in the PR description.

[dmtroyer]

There are some fields that are required, resource_sub_type was the only one that was actually being validated so that's where I started, and I chose to use the name in the csv. A logged on user can have more than one organizer, so they'll probably have to be explicit or select the organizer as part of the upload form or something.

[chadwhitacre]

I'm trying to understand the Opportunity model and the opportunities table. They don't appear to sync up. The latter has way more fields than the former.

class Opportunity < ActiveRecord::Base
  has_many :opportunity_instances
  belongs_to :organizer
  has_one :after_this, class_name: "Opportunity", foreign_key: "after_this_id"
  has_one :before_this, class_name: "Opportunity", foreign_key: "before_this_id"
  belongs_to :resource_sub_type
  has_one :resource_type, through: :resource_sub_type

  validates :resource_sub_type, presence: true

  def active_model_serializer
    OpportunitySerializer
  end
end

Null display is "¤".
Line style is unicode.
Border style is 2.
                                            Table "public.opportunities"
┌───────────────────────┬─────────────────────────────┬────────────────────────────────────────────────────────────┐
│        Column         │            Type             │                         Modifiers                          │
├───────────────────────┼─────────────────────────────┼────────────────────────────────────────────────────────────┤
│ id                    │ integer                     │ not null default nextval('opportunities_id_seq'::regclass) │
│ name                  │ character varying           │                                                            │
│ address               │ text                        │                                                            │
│ description           │ text                        │                                                            │
│ registration_url      │ character varying           │                                                            │
│ location_name         │ character varying           │                                                            │
│ registration_deadline │ timestamp without time zone │                                                            │
│ program_type          │ character varying           │                                                            │
│ logo_url              │ character varying           │                                                            │
│ starts_at             │ timestamp without time zone │                                                            │
│ ends_at               │ timestamp without time zone │                                                            │
│ online_address        │ character varying           │                                                            │
│ zipcode               │ character varying           │                                                            │
│ city                  │ character varying           │                                                            │
│ state                 │ character varying           │                                                            │
│ is_online             │ boolean                     │                                                            │
│ hide_reason           │ character varying           │                                                            │
│ hide                  │ boolean                     │                                                            │
│ contact_name          │ character varying           │                                                            │
│ contact_email         │ character varying           │                                                            │
│ contact_phone         │ character varying           │                                                            │
│ price_level           │ integer                     │                                                            │
│ min_age               │ integer                     │                                                            │
│ max_age               │ integer                     │                                                            │
│ extra_data            │ text                        │                                                            │
│ created_at            │ timestamp without time zone │ not null                                                   │
│ updated_at            │ timestamp without time zone │ not null                                                   │
│ organizer_id          │ integer                     │                                                            │
│ topic_id              │ integer                     │                                                            │
│ after_this_id         │ integer                     │                                                            │
│ before_this_id        │ integer                     │                                                            │
│ badge_class_id        │ character varying           │                                                            │
│ resource_sub_type_id  │ integer                     │                                                            │
└───────────────────────┴─────────────────────────────┴────────────────────────────────────────────────────────────┘
Indexes:
    "opportunities_pkey" PRIMARY KEY, btree (id)
    "index_opportunities_on_after_this_id" btree (after_this_id)
    "index_opportunities_on_before_this_id" btree (before_this_id)
    "index_opportunities_on_organizer_id" btree (organizer_id)
    "index_opportunities_on_resource_sub_type_id" btree (resource_sub_type_id)
    "index_opportunities_on_topic_id" btree (topic_id)
Foreign-key constraints:
    "fk_rails_6d4b8f5561" FOREIGN KEY (topic_id) REFERENCES topics(id)

[chadwhitacre]

A logged on user can have more than one organizer, so they'll probably have to be explicit or select the organizer as part of the upload form or something.

Okay, noted in to-do.

[chadwhitacre]

@dmtroyer Do we have a test suite? How do I run it?

[chadwhitacre]

$ bundle exec rake test
[...]
35 runs, 0 assertions, 0 failures, 35 errors, 0 skips

[dmtroyer]

I'm trying to understand the Opportunity model and the opportunities table. They don't appear to sync up. The latter has way more fields than the former.

Rails is cheeky like that, it dynamically creates default getter and setters for attributes defined in the schema. So, if you're cool with the defaults they don't have to be explicitly defined in the model.

[chadwhitacre]

Okay, thanks for the pointer.

$ head -n12 db/schema.rb
# encoding: UTF-8
# This file is auto-generated from the current state of the database. Instead
# of editing this file, please use the migrations feature of Active Record to
# incrementally modify your database, and then regenerate this schema definition.
#
# Note that this schema.rb definition is the authoritative source for your
# database schema. If you need to create the application database on another
# system, you should be using db:schema:load, not running all the migrations
# from scratch. The latter is a flawed and unsustainable approach (the more migrations
# you'll amass, the slower it'll run and the greater likelihood for issues).
#
# It's strongly recommended that you check this file into your version control system.

[chadwhitacre]

There are some fields that are required

How is this information encoded? I only see three out of 33 columns in the schema that are not null, and I don't see anything suggestive of requirement in opportunity.rb.

[dmtroyer]

How is this information encoded? I only see three out of 33 columns in the schema that are not null, and I don't see anything suggestive of requirement in opportunity.rb.

A couple of different ways:

1. At the database and schema level through migrations
2. At the model level through validations https://github.com/saxifrage/cityasacampus/blob/master/app/models/opportunity.rb#L9

[chadwhitacre]

@dmtroyer I only see one validated (required) field in opportunity.rb. In schema.rb there are only two null: false fields (there is of course an id field in the actual database schema that is also not null): created_at and updated_at. Are those the only three (or four) required fields, then?

The backtrace at #341 (comment) seems to indicate that organizer_id ought to be required as well, and that therefore in general we are not yet requiring all of the fields that we should be.

[dmtroyer]

@whit537 All correct. Field requirement and validation has not been completely flushed out.

[chadwhitacre]

Okay!

[chadwhitacre]

I'm gonna pull @timothyfcook over to make some decisions about required fields for the CSV upload.

[dmtroyer]

!m @whit537 @timothyfcook

[chadwhitacre]

The word from @timothyfcook:

name
description
registration_url
program_type
logo_url
is_online
contact_name
contact_email
contact_phone
price_level
min_age
max_age
organizer_id
topic_id
resource_sub_type_id

If is_online, then online_address should be required. Otherwise, address, city, state, and zipcode should be required.

[chadwhitacre]

[chadwhitacre]

Aren't there supposed to be gems for everything? Isn't that why we use Rails? Let's go look! :-)

https://www.ruby-toolbox.com/categories/CSV_Parsers

[MatthewVita]

dat validation 👍

[dmtroyer]

@whit537 thought I'd help out a little with the uniqueness constraint on name per organizer. Active Record Uniqueness Validations save the day here.

[chadwhitacre]

Blam. Thanks, @dmtroyer. I was just about to ask you for help! :-)

[chadwhitacre]

And with that, I'm out for the rest of the week. Heading to New York for gratipay/inside.gratipay.com#384 this weekend. See you next week! :-)

[dmtroyer]

@whit537 have fun! 🚌 🌆 💵

[chadwhitacre]

/me back ...

[chadwhitacre]

Rebased on master. Former head was c33b567.

[chadwhitacre]

[chadwhitacre]

Working on inline docs.

[chadwhitacre]

I added a dynamic listing of the possible values for topic and resource_sub_type.

[chadwhitacre]

Okay! I think this is ready to merge.

Waddya think, @dmtroyer @timothyfcook? :-)

[timothyfcook]

I'll let @dmtroyer sign off on this one

[chadwhitacre]

Waddya say, @dmtroyer? :-)

[dmtroyer]

Will do sometime today!

On Fri, Nov 27, 2015, 10:25 Chad Whitacre [email protected] wrote:

Waddya say, @dmtroyer https://github.com/dmtroyer? :-)

—
Reply to this email directly or view it on GitHub
#341 (comment)
.

[dmtroyer]

oops, unsure what the previous head was. sorry, seems to be OK. I'll probably squash it before I merge it, anyhow...

[chadwhitacre]

!m @dmtroyer