More consolidation – bug fixing and improvements

August 11, 2007

These bullet points were left over from yesterday, and i’m adding a few more as we go:

  • change what happens if someone adds an article that is already an article or a link: in both cases the story should have 1 point added to its score and the user notified either that ‘a link has been converted into an article’ or ‘the article is already present, you have added to its score’.
  • put in some handling code for going back to ’show’ pages for things that have been deleted – “if @picture = nil” etc
    • This requires some exception handling – about time that i did this in rails!(see below)
    • Done for show picture – need also for story/show and article/show
    • done
  • make pictures act as a link to the show picture page
    • done: code involves wrapping the image_tag as the first parameter to a link_to call:
    • link_to image_tag(“/data/pictures/thumbs/thumbpic#{picture.id}.#{picture.content_type.split(“/”).last}”, :alt => “Missing thumbnail”), :controller => “picture”, :action => “show”, :id => picture.id, :came_from => params[:action]
  • If someone deletes a picture, we need to delete the file as well.
    • I’m going to put a delete method in the model for this, which will take an id and delete the full size and thumbnail pictures with that id. … didn’t work – ah wait, i forgot about the file extension. Now i come to think of it, this should be a class method (ie static) rather than an instance method as we might also want to use it to get rid of left over files that don’t have an object associated with them. Discovered that the way we do class methods in rails(ruby) is to put def Classname.methodname:

#expects options[:id] and options[:extension]
def Picture.delete_from_disc(options = {})
extension = options[:extension]
id = options[:id]
picpath = “#{RAILS_ROOT}/public/data/pictures”
if(File.exists?(“#{picpath}/pic#{id}.#{extension}”))
File.delete(“#{picpath}/pic#{id}.#{extension}”)
end
#now the thumbnail
if(File.exists?(“#{picpath}/thumbs/thumbpic#{id}.#{extension}”))
File.delete(“#{picpath}/thumbs/thumbpic#{id}.#{extension}”)
end
end

I then call that from the picture controller’s delete method, before calling destroy on the db object:

#delete from disc
Picture.delete_from_disc :id => params[:id], :extension => @picture.content_type.split(“/”).last
#delete from db
@picture.destroy

I think that’s the first time i’ve done a rails method that takes any parameters! woot.

One of the points above relates to exception handling: this is nice and easy: here’s how i’m dealing with the case when a requested picture (in show picture) doesn’t exist: found out that you can’t ask for ‘action_name’ in a view, you need to ask for params[:action]. (that reminds me, going to put params output into a debug partial and add it to the _standard layout. OK, done that.)

def show
begin
@picture = Picture.find(params[:id])
rescue ActiveRecord::RecordNotFound
flash[:message] = “Couldn’t find picture##{params[:id]}, redirecting to article list.”
redirect_to :controller => “article”, :action => “list”
end
end

Nice and simple – if we can’t find the picture we go back to the article list. Ideally we could go back to the story the picture belongs to, but that requires knowing the story, which we don’t ordinarily pass through – normally we get the id of the story from the picture’s story_id, but of course we don’t have a picture so we can’t.

That bit of exception handling has got me onto a bit of user testing – found that if you log out while in the ‘edit_story’ page, it crashes looking for an id of nil. Fixed that by putting a bit of checking code in edit_story, which redirects to show_story if the @story.user is different to the session[:user].

Now i have a new, similar problem – going to show picture after doing this, if you hit back it says “no action responded to edit”. Hmm, it should be going back to show, not edit. Let’s have a look. Ah yeah – when i go back from picture, (to story), i wasn’t specifying the controller (which should be ‘story’ of course). Fixed now, along with a bit of other weirdness to do with swapping between show_picture and edit/show story.

Now, where was I? Oh yeah, putting in the exception handling for show_story and show_article. Both will just take you back to the article list if we can’t find the item. OK, that’s done. Looking at ta-da lists, a successful rails app, they have a standard ‘page not found’ page, which has a nice error message and the option to go back. Will look at doing that at some point.

Made a slight change to the show/edit story page – they now show the article that the story is a reply to – this should be useful reference for anyone writing/viewing a story.

  •  Allow user to delete stories – this should delete any associated pictures and movies.

Got this in with a little help from a friend.  It works in quite a cool way:

First we add a :dependent parameter to the has_many: pictures line in the story model.  This says “when a story is destroyed, do ‘something’ to it’s children (the pictures).  In this case the thing we want to do is :destroy  the pictures (the most common use of :dependent).  So, :dependent => :destroy.  “:destroy” is a fixed option symbol, it’s not like a method call into the picture model or controller or anything like that.  It’s like saying “for picture in @story.pictures do picture.destroy”.

However, that’s not enough:  calling destroy on a picture only gets rid of the db record, we need to get rid of the files as well.  We’ve got a method to do that, so in the picture model, we can say ‘before destroying, call this instance method”.  We do that by writing, in picture.rb:

before_destroy :delete_from_disc 

Simple!  Btw, the instance method in picture looks like this:

  def delete_from_disc
picpath = “#{RAILS_ROOT}/public/data/pictures/pic#{self.id}.#{self.content_type.split(“/”).last}”
thumbpath = “#{RAILS_ROOT}/public/data/pictures/thumbs/thumbpic#{self.id}.#{self.content_type.split(“/”).last}”
if File.exists?(picpath)
File.delete(picpath)
end
#now the thumbnail
if File.exists?(thumbpath)
File.delete(thumbpath)
end
end

Note that we don’t call self.destroy – that’s done elsewhere, after this method completes.

Another thing i learned last night was the ‘skinny controller, fat model’ principle.  That states that the controller shouldn’t ever be more than a traffic cop – it shouldn’t do stuff to objects (at least as little as possible anyway), it should just route the program around between other controller, actions and views.  That means that at the moment, where i have in a controller action, some setup code like

story.added_at = DateTime.now.to_s
@story.user_id = session[:user].id
@story.complete = 0

I should really be doing this in the model.  So, I moved that into an initialize method in Story.  Discovered that if you do this you need the line super before doing anything, to call the ActiveRecord initialize method, which does the railsy setup.  However, my next problem is this line:

self.user_id = session[:user].id

In the model, we can’t seem to access the ‘session’ hash – i’m getting an ‘undefined variable’ exception.  After a bit of research it seems like the model deliberately can’t access the session.  So, i guess i’ll need to pass the user through to the constructor.   So, this is the new story constructor:

def initialize(params = {})
super
self.added_at = DateTime.now.to_s
self.user_id = params[:user]
self.complete = 0
end

OK – time to move some more stuff into the story model, like what happens when the story is submitted.  Discovered that even when you do your changes in the model instead of the controller, you still need to call “save” to get them into the database.  New submit method, called by story.create:

  def submit
self.complete = 1
self.points =  1
self.added_at = DateTime.now.to_s
return self.save
end

Again, nothing new, just getting the model to do the work.  Let’s do the same for article controller.

Having problems with this:  here’s my new controller method:

def create
@article = Article.new(params[:article], :user => session[:user])
if @article.save
flash[:notice] = ‘Article was successfully created.’
redirect_to :action => ‘list’
else
render :action => ‘new’
end
end

and the new constructor:

 def initialize(params =(), *options)
super
if self.title == “” && self.url != “”
self.title = (Hpricot(open(@article.url))/”title”).first.inner_html
end
self.added_at = DateTime.now.to_s
self.user_id = :user
self.points = 1
end

I’m having problems getting everything through properly to the constructor.  I think for now i’m going to go back to doing the user_id stuff in the controller and fixing it later when i understand what’s going wrong.

OK – that’s enough controller slimming for now.

Next task:  Check validation code is doing all it should – a picture should have a filename, for example.

Done: Picture validates_presence_of :title, :filename.  Think that’s it for now.

Just one bullet point left to do:

  • change what happens if someone adds an article that is already an article or a link: in both cases the story should have 1 point added to its score and the user notified either that ‘a link has been converted into an article’ or ‘the article is already present, you have added to its score’.

I’ve been thinking about this and i think that any link added to a story should be a fully fledged article as well – there’s no reason not to really.  In order to implement this, all i need to do is to set a link’s score to 1 instead of 0.  Sweet.  Actually, it’s even easier, i just use the article constructor that i made earlier which sets the score to 1 anyway.  Even better.

OK, so next i check whether the article is there already, and if it is then i add a point to it rather than make a new one.

That’s done.  But, i noticed that i can add a link to an article even if it already has it as a link.  So, i need to do a further test after seeing if the article exists already:

      if @original_article.links.include? @article
flash[:message] = “Link already added to this article!”
else
@original_article.links << @article
flash[:message] = “Added article #{@article.id} to article #{@original_article.id}’s links.”
end

This works, but i think this is ‘fat controller’  – i should move this to the model really.  I could probably do something clever with callbacks (the model equivalent of filters).  Hmm.

Anyway, cleanliness aside that’s the bullet points all sorted!  Cool.

This dealing with points has reminded me that i need to get the points system up and running properly so that people can add points to articles.   I need to think about the schema for this a bit:

  • Each user only score a particular article either up or down, once.
  • They can change their mind, and swap n up for a down or vice versa.
  • A user can score many articles.
  • An article can be scored by many users.

So what we’re talking about is a join table between user_id and article_id, which has an extra field that holds the score somehow – either “up” or “down” or -1 or 1, or something.  Hmm, i’ll think about this over the weekend.

Advertisements

One Response to “More consolidation – bug fixing and improvements”


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: