2

My user is submitting a request with multiple items. Regardless of how the items are saved into the SQL database, for easier analytics, I need each item to be parsed out and saved as an individual row of data in a Excel file (Google Drive).

In other words, in SQL, a Request looks like this: {:name => "john", :email => "john@gmail.com", :items => ["a", "b", "c"], but I need this to translate to a document where there are 3 rows for each item, and the only thing different across the rows is the item.

  • Row 1 has Name = John, Email = John@gmail.com, Item = a
  • Row 2 has Name = John, Email = John@gmail.com, Item = b
  • Row 3 has Name = John, Email = John@gmail.com, Item = c

What I have below works, but I am wondering if it is the most efficient, both from the perspective of:

  1. Is there an easier way to get the params in the first place?
  2. Is there an easier way to parse and save the individual data?

Thanks!

MODEL code

class Request < ActiveRecord::Base

  serialize :items
  validate :must_have_one_item

  def must_have_one_item
    errors.add(:items, 'You must select at least one item') unless self.items.detect { |i| i != "0" } 
  end

end

VIEW code

<%= f.check_box(:items, {:multiple => true}, "#{thing}") %>
<%= f.label(:items, "#{thing}") %>

Here, thing is a part of an iterator function that runs through a predefined list of potential items to be selected.

CONTROLLER code with plenty of comments!

class RequestsController < ApplicationController

  def new
    @requestrecord = Request.new
  end

  def create
    @requestrecord = Request.new(request_params) 

    if @requestrecord.save

      # Given model/ form code so far, the items are passed through as an array. The remaining comments uses the example: @requestrecord.items = ["water filter", "tent", "0", "0", "0"]. What happens is that "0" represents blank checkboxes
      @requestrecord.items.select! { |x| x != "0" } #removes blank checkboxes; @requestrecord.items = ["water filter", "tent"]
      num_of_requests = @requestrecord.items.count #counts number of items requested; num_of_requests = 2

      i = 0 
      cloned_request = Hash.new
      while i < num_of_requests do
         cloned_request[i] = Marshal.load(Marshal.dump(@requestrecord)) #creates a cloned_request[0] = @requestrecord and cloned_request[1] = @requestrecord 
         cloned_request[i].items = @requestrecord.items.slice(i) #disaggregates the items into the cloned_requests; cloned_request[0].items = "water filter",  cloned_request[1].items = "tent" 
         i += 1
      end

      cloned_request.each do | key, request |
          request.save_spreadsheet
      end

    else
      render 'new'
    end
  end

  private
    def request_params
      params.require(:request).permit({:items => []})
    end

end
james
  • 3,989
  • 8
  • 47
  • 102

2 Answers2

2

Okay, so there are a bunch of issues here.

First, try to never store arrays of data in the database, as you seem to be doing with your serialize :items. You can read more about why here. What you should do instead is have a model called Item with its own table. From there, you can associate it with other models using more models, such as a UserItem model that has item_id and user_id attributes as integers. This UserItems is your association model and it holds information about how items and users are associated (i.e. which users have which items). If requests and items need to be associated, you can create a RequestItem model too.

When the user submits a list of items, you can take each item from the params[:request][:items] hash and create a record of it with

params[:request][:items].each do |item|
  UserItem.create(user_id: @user.id, item_id: item.to_i)
end

Note that it says item.to_i. This is because, once you have your Items table, each item will have an id. Once you change your views to send you the item_id (instead of item name), you can easily create UserItems for it using the above logic. (to_i is used because most of the time data is sent as a string, which you will need to convert to an integer).

Second, you really should move your business logic to its appropriate model, or at least a helper. As a principle, your controllers should be "skinny" and your models should be "fat." There are many reasons for this, but in my opinion the most important one is that it makes your code much easier to test. You can read more about this here.

Community
  • 1
  • 1
Ege Ersoz
  • 6,461
  • 8
  • 34
  • 53
  • Hi thanks for posting! On the second point, yes I totally agree... I was going to move it out later, thanks for the catch. On the first point, thanks so much for letting me know! What you're saying definitely makes sense, and I'll think on it further. I'm also thinking about when I need this data and whether I want to add another table (there are several already) to keep track of. – james May 26 '14 at 06:24
  • There really is no limit to the number of tables you can have. A table is just a place to organize information. As long as you don't duplicate information across tables (e.g. two tables serving similar purposes), feel free to create them as necessary. – Ege Ersoz May 26 '14 at 06:47
0

Agree with Enraged Camel - I think you can make your create method even more efficient:

#app/controllers/requests_controller.rb
Class RequestsController < ApplicationController
    def create
        items = params[:request][:items]
        if items.kind_of?(Array)
            #means there are multiple array items
            for item in items do
               request = Request.new(your: item[:attribute], and: item[:another_attribute])
               request.save
            end
        else
            request = Request.new(request_params)
            request.save
        end
    end
    private

    def request_params
       request.require(request).permit(:your, :attributes)
    end
end

Don't mean to steal anyone's thunder; just hope you get some more ideas from what I've posted!

Richard Peck
  • 76,116
  • 9
  • 93
  • 147