has_many through question

I have made 3 tables

+------------+ +----------------+ +------------+

people | |people_campaigns| |campaigns |

+------------+ +----------------+ +------------+

id :int | <----> |people_id :int | /-> |id :int | name :string| |campaign_id :int| <-/ |name :string| etc ... | |exported :int | |desc :string|

+------------+ +----------------+ +------------+

The people model looks like this

has_many :people_campaigns has_many :campaigns, :through => :people_campaigns

the other models I left empty because there is no need for me to work the other way I think

What I want to do is make an export of all people that are in campaign "x" and are not yet exported. If they are exported then update with people_campaigns.exported=1

what I did is (except for the fastercsv lines):

people = Person.find(:all) people.each do |person|   campaigns = person.campaign   campaings.each do |campaign|     if campaign.name == params[:campaignName]       tester = PeopleCampaign.find(:first,                                    :conditions => ["person_id=? and campaign_id=? and isnull(exported)",person.id,campaign.id]                                    )       if tester         #print all to fastercsvlines         tester.exported = 1         tester.save       end     end   end end

This part is out of the top of my head but I don't like this code and I think there is a nicer and better (ruby on rails) way to do this ??? especially the part where i lookup the peoplecampaign record for which i had to add an id (yeah didn't mention it in the table layout)

One thing which I notice is that you have two options:

has_many :campaigns, :through => relationships

(choose a better model name than "relationship")

Or:

has_and_belongs to :campaigns_people

with campaigns first.

At least, that's my understanding. I don't think that there's anything technically wrong for a hmt relationship to use "campaigns_people", but it's not the convention.

-Thufir

My main problem is with the part that it loops twice and does a seperate lookup. I am worried that this had major performance problems because the Person table will hold 1.2 million records and at least 700.000 people per campaigns there will be 4 at least this means that the people_campaigns table will grow to about 2.8 million records and perhaps more

The naming convention I got from the rails wiki but I will look into this however this isn't really the problem

Let's set this up conventionally:

class Person < ActiveRecord::Base    has_many :campaigns :through => :campaign_enrollments    has_many :campaign_enrollments

   # The people table needs at least id as well as any other attributes. end

class CampaignEnrollment    belongs_to :person    belongs_to :campaign    # The campaign_enrollments table needs at least id, person_id, campaign_id and exported fields. end

class Campaign    has_many :campaign_enrollments    has_many :people :through => :campaign_enrollments    # the campaigns table needs at least id field plus other campaign attributes. end

Now given a campaign

   campaign = Campaign.find(...)

   campaign.campaign_enrollments.find(:all, :include => [:people, ;campaigns] :conditions => ["exported !=?", true]).each do | campaign_enrollment|       # whatever exporting does here      campaign_enrollment.update_attribute(:exported, true)   end

[...]

   campaign.campaign_enrollments.find(:all, :include => [:people, ;campaigns] :conditions => ["exported !=?", true]).each do | campaign_enrollment|       # whatever exporting does here      campaign_enrollment.update_attribute(:exported, true)   end

[...]

This sort of iteration couldn't go in the controller, could it? It would have to go in the view?

-Thufir

If I understood it correctly.. almost eveything that can be in the controller can be done in the view (with some exceptions)

however this example is being used for an import and export the export doesn't have a view I use fastercsv to generate a csv file streaming to the browser..

Thufir wrote:

My main problem is with the part that it loops twice and does a
seperate lookup. I am worried that this had major performance problems because the Person table will hold 1.2 million records and at least 700.000 people per campaigns there will be 4 at least this means that the people_campaigns table will grow to about 2.8 million records and perhaps more

The naming convention I got from the rails wiki but I will look into this however this isn't really the problem

Let's set this up conventionally:

class Person < ActiveRecord::Base   has_many :campaigns :through => :campaign_enrollments   has_many :campaign_enrollments

  # The people table needs at least id as well as any other
attributes. end

class CampaignEnrollment   belongs_to :person   belongs_to :campaign   # The campaign_enrollments table needs at least id, person_id, campaign_id and exported fields. end

class Campaign   has_many :campaign_enrollments   has_many :people :through => :campaign_enrollments   # the campaigns table needs at least id field plus other campaign
attributes. end

Now given a campaign

  campaign = Campaign.find(...)

  campaign.campaign_enrollments.find(:all, :include => [:people, ;campaigns] :conditions => ["exported !=?", true]).each do | campaign_enrollment|      # whatever exporting does here     campaign_enrollment.update_attribute(:exported, true) end

it might be nice to add

has_many :unexported_enrollments, :conditions => ["exported != ?",
true], :class_name => "CampaignEnrollment" to campaign.

you could then write

campaign.unexported_enrollments.find :all, :include => [...] do | campaign_enrollment| ... end

This will do exactly the same thing, but can make things so much more
readable.

Fred

But it really should be done in the model, with a method used to allow the controller to trigger it.

Putting too much business logic in a controller is a bad code smell.

Putting too much logic in views is an even bigger bad code smell.

http://weblog.jamisbuck.org/2006/10/18/skinny-controller-fat-model

it might be nice to add has_many :unexported_enrollments, :conditions => ["exported != ?", true], :class_name => "CampaignEnrollment" to campaign.

or even

  has_many :enrollments, :class_name => "CampaignEnrollment" do        def unexported            find(:all, :conditions => ["exported !=>", true]        end

Which would allow

   campaign.enrollments.unexported.each do | enrollment | ...

   has_many :enrollments