CSC/ECE 517 Spring 2015/oss E1504 IMV

E1504. Refactoring the Bookmark Model

This page provides a description of the Expertiza based OSS project. This team project successfully refactored the Bookmark Model by removing the duplicate code, combining the methods, improving the search function, moving Bmapping model methods to their respective class, covering dependencies associated with changed segments and adding a User Interface for using bookmarks.

Introduction to Expertiza

Expertiza is a large project developed as a combined effort of students and faculty using the Ruby on Rails framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages.

Expertiza is supported by National Science Foundation under Grant No. 0536558. Additional funding from the NCSU Learning in a Technology-Rich Environment (LITRE) program, the NCSU Faculty Center for Teaching and Learning, the NCSU STEM Initiative, and the Center for Advanced Computing and Communication.

Problem Statement

Classes involved:

bookmark.rb
bmapping.rb
bookmarks_controller.rb
auth_controller.rb
auth_helper.rb

What they do: The bookmark model and controller work together to create and maintain user specific bookmarks, which can be added to different topics. Moreover, the user that created a bookmark can use his privilege to edit it at a later point in time. As a functionality added for convenience, any user can search bookmarks by either users who created them or their bookmark tags.

What needs to be done:

• The search methods in bookmarks model are being used used on a very granular level. This led to redundancy in bookmarks search methods. These methods include search_alltags_allusers, search_fortags_allusers, search_fortags_forusers, search_alltags_forusers. Hence duplicates in these methods are to be singled out.
• Following the CRUD declaration in Ruby on Rails, where the method name should specify the pseudo use of itself,methods such as add_new_bookmark and edit_this_bookmark have violate this naming convention. Therefore, we need to change the names of these two methods to create and edit, respectively. Upon changing the names, all the dependencies need to be updated, as well.
• Refactor add_topic_bookmark and add_this_bookmark by removing the repetitive code. These two methods differ only in 2 lines of code, which make it reasonable to merge them together into a single method that provides functionality of both individual methods.
• Fix the bug in add_bookmark method where only bookmarks without topic_id can be properly created. We add the functionality to this method to allow it to also create bookmarks with provided topic_id.
• Methods add_bmapping and add_bmapping_signuptopic need to be moved to their appropriate model (Bmapping model) rather than reside in Bookmarks model. Completing this task involves checking the dependencies and updating the function calls for these methods, accordingly.
• Add user interface for Bookmarks as this functionality has not been a part of Expertiza
• Create a functional tests suite covering the functionality of all the methods that have been changed by our team.
• Create a Cucumber integration test to show the functionality of added user interface for Bookmarks.

Design Patterns Followed

Single Responsibility Principle:

As per Single Responsibility Principle, every class should have responsibility over a single part of functionality provided by software and that responsibility should be entirely encapsulated by the class. All the services and operations within the class should be aligned with that responsibility. Following this principle, we found that methods add_bmapping and add_bmapping_signuptopic were present in the Bookmark model, while they actually belong to Bmapping model. So we moved those methods to the Bmapping model and took care of all the dependencies. We created functional tests for these methods to prove that refactoring was successful.

Polymorphism:

We realized that the methods search_fortags_allusers,search_fortags_forusers could be combined into one method. Similarly, search_alltags_allusers and search_alltags_forusers could be grouped together into a single method. The reason for this is because these methods, essentially, offer the same functionality, but using different parameters. So, we wrote a common method which takes parameters and, based on the parameters passed, it provides the required functionality.

Changes Made

Bookmark Model

Method Name	Changes Made	Reason For Change
add_this_bookmark	Added functionality of add_topic_bookmark	To implement code reusability and remove repetitive code.
add_topic_bookmark	Merged the functionality into add_this_bookmarks and deleted add_topic_bookmark	To implement code reusability and remove repetitive code.
add_bookmark	Added a line that checks whether topic_id has been provided in the request	It allows creation of new Bmapping if no topic_id was given and removes the bug of searching for bookmark with nil topic_id later on
search_alltags_allusers	Merged search_alltags_allusers and search_alltags_foruser functions into search_alltags	Duplicate code present in both the functions. They differed only with the usage of parameters passed.
search_alltags_foruser	Merged search_alltags_allusers and search_alltags_foruser functions into search_alltags function	Duplicate code present in both the functions. They differed only with the usage of parameters passed.
search_fortags_allusers	Merged search_fortags_allusers and search_fortags_foruser functions into search_fortags function	Duplicate code present in both the functions. They differed only with the usage of parameters passed.
search_fortags_foruser	Merged search_fortags_allusers and search_fortags_foruser functions into search_fortags function	Duplicate code present in both the functions. They differed only with the usage of parameters passed.
add_new_bookmark	Changed the method name to "create"	It is a much more meaningful and simpler name for a method that creates a new bookmark
edit_this_bookmark	Changed the name to "edit"	It is a much more appropriate name for a method that edits the bookmark it was called on
add_bmapping	Moved to class 'Bmapping'	This method helps in creating new bmapping. Hence it makes more sense if it belongs to bmapping class itself.
add_bmapping_signuptopic	Moved to class 'Bmapping'	This method helps in adding a signup topic to a newly created bmapping. Hence it makes more sense if it belongs to bmapping class itself.

Bookmark Controller

Method Name	Changes Made	Reason For Change
view_topic_bookmarks	Renamed Bmappingstags to Bmappingstag	Due to incorrect class name, the function was not being accessed.
view_review_bookmarks	Renamed Bmappingstags to Bmappingstag	Due to incorrect class name, the function was not being accessed.
view	Renamed Bmappingstags to Bmappingstag	Due to incorrect class name, the function was inaccessible.
edit_bookmark_form	Renamed Bmappingstags to Bmappingstag	Due to incorrect class name, the function was inaccessible.
create_tag_bookmark	Renamed Bmappingstags to Bmappingstag	Due to incorrect class name, the function was inaccessible.
edit_bookmark	Renamed edit_this_bookmark to edit	The method name has been refactored to edit as per the requirement
add_bookmark	Renamed add_topic_bookmark to add_this_bookmark	Due to incorrect method name, the method was inaccessible

Views

Method Name	Changes Made	Reason For Change
bookmarks/_result.html.erb	removed asctime from the below line	Was throwing error
bookmarks/search_bookmarks.html.erb	removed unused code	not used
bookmarks/view_bookmarks.html.erb	Changed line to <%= link_to('Back', session[:comingFrom]) %>	To make the back link work
bookmarks/_searchmine.html.erb	changed line to<%= form_tag(:action => "search_bookmarks") do%>	% was used instead of %= and it was throwing error
bookmarks/add_bookmark_form.html.erb	<%= form_tag(:action => "add_bookmark", :method => "post") do %>	% used instead of %= and it was not working
bookmarks/managing_bookmarks.html.erb	added partials	Nothing was there before to display
tree_display/list.html.erb	added a link to Manage Bookmarks page	This allows a user to see the link to Managing Bookmarks page as soon as the user logs in.

Re-factored Code Cases

Case 1 : Refactoring add_this_bookmark and add_topic_bookmark

The consistency of these two methods shows plenty of repetitive code. In order to effectively refactor these methods and reuse the code they both need, our team merged the two methods together by enabling add_this_bookmark method to handle creation of a bookmark when a topic id is provided (main functionality of add_topic_bookmark method). Once we implemented this change on add_this_bookmark method, we deleted add_topic_bookmark method as it became obsolete. Therefore, we retained the same functionality while reducing the method by 7 repetitive lines of code.

Before Changes

Please note that these two methods only differ in a single line of code (line 6 and 16) where line 6 takes an extra parameter (topic_id).

 1:  def self.add_topic_bookmark(b_url, b_title, b_tags_text, b_description,session_user, topicid)
 2:    # Check if the bookmark exists and add / edit based on that
 3:    bookmark_exists = check_bookmark_exists(b_url)
 4:    bmapping_exists = check_bmapping_exists(b_url,session_user)
 5:    if (!bookmark_exists || !bmapping_exists)
 6:      Bookmark.add_bookmark(b_url, b_title, b_tags_text, b_description,session_user,topicid)
 7:    elsif (bmapping_exists)
 8:      Bookmark.edit_this_bookmark(b_url, b_title, b_tags_text, b_description,session_user)
 9:    end
10:  end
11:
12:  def self.add_this_bookmark(b_url, b_title, b_tags_text, b_description,session_user)
13:    bookmark_exists = check_bookmark_exists(b_url)
14:    bmapping_exists = check_bmapping_exists(b_url,session_user)
15:    if (!bookmark_exists || !bmapping_exists)
16:      Bookmark.add_bookmark(b_url, b_title, b_tags_text, b_description,session_user)
17:    elsif (bmapping_exists)
18:      Bookmark.edit_this_bookmark(b_url, b_title, b_tags_text, b_description,session_user)
19:    end
20:  end

After Changes

 1:  #Adds bookmark with and without topic_id
 2:  def self.add_this_bookmark(b_url, b_title, b_tags_text, b_description,session_user, topic_id)
 3:    # first check existence of the given bookmark and bmapping name
 5:    bookmark_exists = check_bookmark_exists(b_url) 
 6:    bmapping_exists = check_bmapping_exists(b_url,session_user)
 7:    if (!bookmark_exists || !bmapping_exists) # they don't exists, so create new ones
 8:      if(topic_id) # topic_id has been provided
 9:        Bookmark.add_bookmark(b_url, b_title, b_tags_text, b_description,session_user,topic_id)
10:      else # topic_id has not been provided
11:        Bookmark.add_bookmark(b_url, b_title, b_tags_text, b_description,session_user, nil)
12:      end
13:    elsif (bmapping_exists) # if the bmapping already exists, edit it instead of creating a new one
14:      Bookmark.edit(b_url, b_title, b_tags_text, b_description,session_user)
15:    end
16:  end

Case 2 : Fixing a bug in add_bookmark method

Our team fixed the bug associate with topic_id nil being provided to add_bookmark method (user has not specified topic_id) when creating a bookmark. We include an if statement that calls add_bmapping_signuptopic method only if the provided topic_id is not nil. This change has been implemented on line 9 in the code block shown bellow.

Before changes

Parts such as bookmark_resource.nil? need to be refactored to a shorter version !bookmark_resource. Similar opportunity to refactor comes up 2 more times within this code block ( !bmapping.nil? , !topic.nil?). We also need to change the first if block to enable signup topic to be added only if topic_id does not already exists. Doing this will fix the bug of signup_topic overwriting an existing topic_id in case if topic was already created in the past.

  # Adds a bookmark and its various associations
    def self.add_bookmark(b_url, b_title, b_tags_text, b_description,session_user,topicid)
      bookmark_resource = Bookmark.where(["url = ?",b_url]).first

      # Bookmark with the same url does not exists.
      if bookmark_resource.nil?
        # Add the newly discovered bookmark
        bookmarkid = add_new_bookmark(b_url,session_user.id)
        # Add its associations to a user
        bmappingid = add_bmapping(bookmarkid, b_title, session_user.id, b_description,b_tags_text )
        # Add its association to the sign up topic
        add_bmapping_signuptopic(topicid, bmappingid)

        # Bookmark with the same url exists.
      else
        bmapping = Bmapping.where(bookmark_id: bookmark_resource.id, user_id: session_user.id).first
        # Bookmark with the same user - bmapping exists.
        if ( !bmapping.nil? )
          topic = SignUpTopic.find(topicid)
          if( !topic.nil? )
            topic.bmappings.each do |mapping|
              if (mapping.id == bmapping.id)
                Bookmark.edit_this_bookmark(b_url, b_title, b_tags_text, b_description,session_user)
              end
            end

            # Signup Topic does not exists
          else
            add_bmapping_signuptopic(topicid, bmapping.id)
          end

          # Bookmark with same user - bmapping does not exists.
        else
          # Increment user count for the existing bookmark
          bookmark_resource.user_count = bookmark_resource.user_count + 1
          bookmark_resource.save
          # Add its association with the user
          bmappingid = add_bmapping(bookmark_resource.id, b_title, session_user.id, b_description,b_tags_text)
          add_bmapping_signuptopic(topicid, bmappingid)
        end
      end
    end

After Changes

Once the changes described in the Before Change section were carried out, the code looks cleaner, the bug with topic_id is removed and the code functions marvelously!

# Adds a bookmark and its various associations
    def self.add_bookmark(b_url, b_title, b_tags_text, b_description,session_user,topic_id)
      bookmark_resource = Bookmark.where(["url = ?",b_url]).first

      # Bookmark with the same url does not exists.
      if !bookmark_resource # if bookmark resource is nil (does not exist)
        # Add the newly discovered bookmark
        bookmarkid = create(b_url,session_user.id)
        # Add its associations to a user
        bmappingid = Bmapping.add_bmapping(bookmarkid, b_title, session_user.id, b_description,b_tags_text )
        if(topic_id) # if topic_id was provided, add the signup topic
          # Add its association to the sign up topic if the topic was provided
          Bmapping.add_bmapping_signuptopic(topic_id, bmappingid)
        end
        # Bookmark with the same url exists.
      else
        bmapping = Bmapping.where(bookmark_id: bookmark_resource.id, user_id: session_user.id).first
        # Bookmark with the same user - bmapping exists.
        if (bmapping) # if bmapping exists (it is not nil), look it up and edit the bookmarks
          topic = SignUpTopic.find(topic_id)
          if(topic) # if topic exists (it is not nil) use it to edit bookmark
            topic.bmappings.each do |mapping|
              if (mapping.id == bmapping.id)
                Bookmark.edit(b_url, b_title, b_tags_text, b_description,session_user)
              end
            end

            # Signup Topic does not exists
          else
            Bmapping.add_bmapping_signuptopic(topic_id, bmapping.id)
          end

          # Bookmark with same user - bmapping does not exists.
        else
          # Increment user count for the existing bookmark
          bookmark_resource.user_count = bookmark_resource.user_count + 1
          bookmark_resource.save
          # Add its association with the user
          bmappingid = Bmapping.add_bmapping(bookmark_resource.id, b_title, session_user.id, b_description,b_tags_text)
          Bmapping.add_bmapping_signuptopic(topic_id, bmappingid)
        end
      end
    end

Case 3 : Refactoring search_fortags_allusers and search_fortags_forusers

Both these methods contain almost the same code except for few conditional lines. search_fortags_allusers searches for all tags for all users whereas search_fortags_forusers searches only for those tags that belong to the user. The code for both these functions differed only in the query clause where at one place it checks for records returned for a specific user and at the other place, it does not have this check.

Before Changes: search_fortags_allusers function

def  self.search_fortags_allusers(tags_array, order_by)

      result_array = Array.new # returns this array
      ## find the tags associated with these tagnames
      @tags = BookmarksHelper.find_tags(tags_array)
      @q_tuples_with_all_tags = Array.new
      ## retreive mapping_ids taggeed with all of the word tags
      ## for every word, search for every bookmark that was tagged with that word. Then take the intersection of all the bmapping_ids
      first_time = "true"
      for each_tag in @tags

        q_tuples = BmappingsTags.where(["tag_id = ?", each_tag])

        if first_time == "true"
          for q_t in q_tuples
            @q_tuples_with_all_tags << q_t.bmapping_id
          end

          first_time = "false"
        else
          temp_array = Array.new
          for q_t in q_tuples
            temp_array << q_t.bmapping_id
          end
          @q_tuples_with_all_tags = @q_tuples_with_all_tags & temp_array ## returns the items  common to both arrays
        end

      end
      ## now you have qualifer tuples with all the required bmapping ids - search for the req ones
      temp_result_records =  Bmapping.where(["id in (?)", @q_tuples_with_all_tags])
      result_records = Array.new
      ## organize these tuples in the order of most earliest, most popular
      if (order_by =="most_recent")
        result_records = temp_result_records.sort {|x,y| y.date_created <=> x.date_created}
      else
        result_records = temp_result_records.sort {|x,y| y.bookmark.user_count <=> x.bookmark.user_count }
      end
      ## for evey bmapping_id, create a hash, and push into the result_array
      for result in result_records
        result_hash = Hash.new
        result_hash["id"] = result.id
        result_hash["url"] = result.bookmark.url
        result_hash["created_at"] = result.date_created
        result_hash["user"] = result.user.name
        result_hash["title"] = result.title
        result_hash["description"] = result.description
        result_hash["copied_by"] = result.bookmark.user_count
        ## now retrieving tags for this user-bookmak mapping
        ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
        ## Append all these into a comma separated string, and push them onto the hash

        tag_fetchs = BmappingsTags.where(["bmapping_id =?",result.id])
        tag_array = Array.new
        for tag_fetch in tag_fetchs
          tag_array <<  Tag.find(tag_fetch.tag_id).tagname
        end
        result_hash["tags"]  = BookmarksHelper.join_tags(tag_array)
        result_array << result_hash
      end
      return result_array
    end

Before Changes: search_fortags_foruser function

 def self.search_fortags_foruser(tags_array, this_user_id, order_by)
      #order by is in "most_popular" and "most_recent"
      result_array = Array.new


      ## search for ids of the tags
      @tags = BookmarksHelper.find_tags(tags_array)
      @q_tuples_with_all_tags = Array.new
      first_time = "true"
      for each_tag in @tags
        ##search for all qualifier tuples with b
        q_tuples = BmappingsTags.where(["tag_id = ?", each_tag])
        #for q_t in q_tuples
        #end

        if first_time == "true"
          for q_t in q_tuples
            @q_tuples_with_all_tags << q_t.bmapping_id
          end

          first_time = "false"
        else
          temp_array = Array.new
          for q_t in q_tuples
            temp_array << q_t.bmapping_id
          end
          @q_tuples_with_all_tags = @q_tuples_with_all_tags & temp_array ## returns the items  common to both arrays
        end
      end


      ## now you have qualifer tuples with all the required bmapping ids - search for the req ones
      temp_result_records =  Bmapping.where(["id in (?) and user_id = ?", @q_tuples_with_all_tags,this_user_id ])

      ## organize these tuples in the order of most earliest, most popular
      result_records = Array.new
      if (order_by == "most_recent")
        result_records = temp_result_records.sort {|x,y| y.date_created <=> x.date_created}
      else
        result_records = temp_result_records.sort {|x,y| y.bookmark.user_count <=> x.bookmark.user_count }
      end


      for result in result_records
        result_hash = Hash.new
        result_hash["id"] = result.id
        result_hash["url"] = result.bookmark.url
        result_hash["user"] =  User.find(this_user_id).name
        result_hash["title"] = result.title
        result_hash["description"] = result.description
        result_hash["copied_by"] = result.bookmark.user_count
        result_hash["created_at"] = result.date_created
        ## now retrieving tags for this user-bookmak mapping
        ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
        ## Append all these into a comma separated string, and push them onto the hash

        tag_fetchs = BmappingsTags.where(["bmapping_id=?",result.id])
        tag_array = Array.new
        for tag_fetch in tag_fetchs
          tag_array <<  Tag.find(tag_fetch.tag_id).tagname
        end
        result_hash["tags"]  = BookmarksHelper.join_tags(tag_array)
        result_array << result_hash
      end

      return result_array
    end

After Changes

The above two functions are merged into a single function named "search_fortags function" as shown below. If we want to search for specified tags for all users, the userid is passed as nil. If we want to search for the specified tags belonging to a particular userid, we pass that userid.

      ## searches for tspecified tags, among a specified user, orders them by most popular and the most recently added
    def self.search_fortags(tags_array, order_by,this_user_id)
      #order by is in "most_popular" and "most_recent"
      result_array = Array.new


      ## search for ids of the tags
     @tags = BookmarksHelper.find_tags(tags_array)

      @q_tuples_with_all_tags = Array.new
      first_time = "true"
      for each_tag in @tags
        ##search for all qualifier tuples with b
        q_tuples = BmappingsTag.where(["tag_id = ?", each_tag])
        

        #if first_time == "true"
          for q_t in q_tuples
            @q_tuples_with_all_tags << q_t.bmapping_id
          end

          #first_time = "false"
        # else
        #   temp_array = Array.new
        #   for q_t in q_tuples
        #     temp_array << q_t.bmapping_id
        #   end
        #   @q_tuples_with_all_tags = @q_tuples_with_all_tags & temp_array ## returns the items  common to both arrays
        # end
      end
      
      ##If the user id is passed, it will not be nil. So we use it in the query as shown below.
      if(this_user_id)
        ## now you have qualifer tuples with all the required bmapping ids and the requested userid- search for the req ones
        temp_result_records =  Bmapping.where(["id in (?) and user_id = ?", @q_tuples_with_all_tags, this_user_id ])
      else
        ## now you have qualifer tuples with all the required bmapping ids - search for the req ones
        temp_result_records =  Bmapping.where(["id in (?)", @q_tuples_with_all_tags])
      end
      
      ## organize these tuples in the order of most earliest, most popular
      result_records = Array.new
      if (order_by == "most_recent")
        result_records = temp_result_records.sort {|x,y| y.date_created <=> x.date_created}
      else
        result_records = temp_result_records.sort {|x,y| y.bookmark.user_count <=> x.bookmark.user_count }
      end
     

      for result in result_records
        result_hash = Hash.new
        result_hash["id"] = result.id
        result_hash["url"] = result.bookmark.url
        result_hash["user"] =  result.user.name
        result_hash["title"] = result.title
        result_hash["description"] = result.description
        result_hash["copied_by"] = result.bookmark.user_count
        result_hash["created_at"] = result.date_created
        ## now retrieving tags for this user-bookmak mapping
        ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
        ## Append all these into a comma separated string, and push them onto the hash

        tag_fetchs = BmappingsTag.where(["bmapping_id=?",result.id])
        tag_array = Array.new
        for tag_fetch in tag_fetchs
          tag_array <<  Tag.find(tag_fetch.tag_id).tagname
        end
        result_hash["tags"]  = BookmarksHelper.join_tags(tag_array)
        result_array << result_hash
      end
      
      return result_array
    end

Before Changes:search_alltags_foruser function

 def self.search_alltags_foruser (the_userid, order_by)
        result_array = Array.new #going to append all the results into this array

        if(order_by == "most_recent" )
          ## find all the bmappings in this system, created by this user, order them by the date created.
          # For each tuple returned from the bmapping, generate a hash, containing the url, the specified user's name, date this mapping was made,
          ## title, and description provided by this user. Store these hashes sequentially in a array ad return the array

          result_records = Bmapping.where([" user_id = ?", the_userid]).order("date_created DESC").limit(20)
          for result in result_records
            result_hash = Hash.new
            result_hash["id"] = result.id
            result_hash["url"] = result.bookmark.url
            result_hash["user"] = User.find(the_userid).name
            result_hash["created_at"] = result.date_created
            result_hash["title"] = result.title
            result_hash["description"] = result.description
            result_hash["copied_by"] = result.bookmark.user_count
            ## now retrieving tags for this user-bookmak mapping
            ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
            ## Append all these into a comma separated string, and push them onto the hash

            tag_fetchs = BmappingsTags.where(["bmapping_id = ?",result.id])
            tag_array = Array.new
            for tag_fetch in tag_fetchs
              tag_array << Tag.find(tag_fetch.tag_id).tagname
            end
            result_hash["tags"] = BookmarksHelper.join_tags(tag_array)
            result_array << result_hash
          end
        elsif ( order_by == "most_popular")
          ### retrieving the most popular records that this user.(The user need not be the discoverer). First retrieve the the user's bookmarks from the bmapping.
          ### order these records, based on the user_count of the url
          temp_result_records = Bmapping.where([" user_id = ?", the_userid])
          ## order result records by result.user_count a.sort {|x,y| y <=> x }
          result_records = temp_result_records.sort {|x,y| y.bookmark.user_count <=> x.bookmark.user_count}
          for result in result_records
            result_hash = Hash.new
            result_hash["url"] = result.bookmark.url
            result_hash["id"] = result.id
            result_hash["user"] = User.find(the_userid).name
            result_hash["title"] = result.title
            result_hash["description"] = result.description
            result_hash["copied_by"] = result.bookmark.user_count
            ## now retrieving tags for this user-bookmak mapping
            ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
            ## Append all these into a comma separated string, and push them onto the hash

            tag_fetchs = BmappingsTags.where(["bmapping_id = ?",result.id])
            tag_array = Array.new
            for tag_fetch in tag_fetchs
              tag_array << Tag.find(tag_fetch.tag_id).tagname
            end
            result_hash["tags"] =BookmarksHelper.join_tags(tag_array)
            result_array << result_hash
          end
        end
        return result_array
      end

Before Changes : search_alltags_allusers function

def self.search_alltags_allusers(order_by)
      result_array = Array.new #going to append all the results into this array


      if(order_by == "most_recent")
        ## find all the records in the system, order them by the date created. Using Bmapping here. Returns mappings that where created most recently,
        ## the user that created this mapping, the title and description provided this user
        result_records = Bmapping.all(:order =>"date_created DESC", :limit =>20)

        for result in result_records
          ## for each tuple returned by the query above, create a new hash, store the values appropriately, and append into the return_array
          result_hash = Hash.new
          result_hash["id"] = result.id
          result_hash["url"] = result.bookmark.url
          result_hash["user"] = result.user.name # displaying the newest owner of the bookmark,
            result_hash["title"] = result.title
          result_hash["created_at"] = result.date_created
          result_hash["description"] = result.description
          result_hash["copied_by"] = result.bookmark.user_count ## number of people having this user in their repository
          ## now retrieving tags for this user-bookmak mapping
          ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
          ## Append all these into a comma separated string, and push them onto the hash
          tag_fetchs = BmappingsTags.where(["bmapping_id = ?",result.id])
          tag_array = Array.new
          for tag_fetch in tag_fetchs
            tag_array << Tag.find(tag_fetch.tag_id).tagname
          end
          result_hash["tags"] = BookmarksHelper.join_tags(tag_array)
          result_array << result_hash

        end
      elsif (order_by == "most_popular")
        ## returns the url boomarked by the most number of users, the discoverer of that url, the title and description provided by the discoverer
        result_records = Bookmark.all(:order =>"user_count DESC", :limit =>20)
        for result in result_records
          ## for each tuple returned by the query above, create a new hash, store the values appropriately, and append into the return_array
          result_hash = Hash.new
          result_hash["url"] = result.url
          result_hash["user"] = User.find(result.discoverer_user_id).name
          result_hash["copied_by"] = result.user_count
          b_u_mapping = Bmapping.where(["bookmark_id = ? and user_id = ?", result.id, result.discoverer_user_id]).first
          result_hash["id"] = b_u_mapping.id
          result_hash["description"] = b_u_mapping.description
          result_hash["title"] = b_u_mapping.title
          ## now retrieving tags for this user-bookmak mapping
          ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
          ## Append all these into a comma separated string, and push them onto the hash

          tag_fetchs = BmappingsTags.where(["bmapping_id = ?",b_u_mapping.id])
          tag_array = Array.new
          for tag_fetch in tag_fetchs
            tag_array << Tag.find(tag_fetch.tag_id).tagname
          end
          result_hash["tags"] = BookmarksHelper.join_tags(tag_array)
          result_array << result_hash
        end
      end
      return result_array
    end

After Changes

The above two functions were merged into a single function "search_alltags" as shown below. If we want to search for all tags for all users, the userid is passed as nil. If we want to search for all tags belonging to a particular userid, we pass that userid.

def self.search_alltags (the_userid, order_by)

        result_array = Array.new #going to append all the results into this array

        if(order_by == "most_recent" )
          ## find all the bmappings in this system, created by this user, order them by the date created.
          # For each tuple returned from the bmapping, generate a hash, containing the url, the specified user's name, date this mapping was made,
          ## title, and description provided by this user. Store these hashes sequentially in a array ad return the array
 
          ## If no user id is passed, the search is for all the user ids
          if(!the_userid)
            ## find all the records in the system, order them by the date created. Using Bmapping here. Returns mappings that where created most recently,
            ## the user that created this mapping, the title and description provided this user
            
            result_records = Bmapping.all.order("date_created DESC").limit(20)
          else
            result_records = Bmapping.where([" user_id = ?", the_userid]).order("date_created DESC").limit(20)
          end
          
          #result_records = Bmapping.where([" user_id = ?", the_userid]).order("date_created DESC").limit(20)
          for result in result_records
            result_hash = Hash.new
            result_hash["id"] = result.id
            result_hash["url"] = result.bookmark.url
            result_hash["user"] = result.user.name
            result_hash["created_at"] = result.date_created
            result_hash["title"] = result.title
            result_hash["description"] = result.description
            result_hash["copied_by"] = result.bookmark.user_count
            ## now retrieving tags for this user-bookmak mapping
            ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
            ## Append all these into a comma separated string, and push them onto the hash

            tag_fetchs = BmappingsTag.where(["bmapping_id = ?",result.id])
            tag_array = Array.new
            for tag_fetch in tag_fetchs
              tag_array << Tag.find(tag_fetch.tag_id).tagname
            end
            result_hash["tags"] = BookmarksHelper.join_tags(tag_array)
            result_array << result_hash

          end
        elsif ( order_by == "most_popular" and the_userid) ## Most popular tags for the supplied user
          ### retrieving the most popular records that this user.(The user need not be the discoverer). First retrieve the the user's bookmarks from the bmapping.
          ### order these records, based on the user_count of the url
          temp_result_records = Bmapping.where([" user_id = ?", the_userid])
          ## order result records by result.user_count a.sort {|x,y| y <=> x }
          result_records = temp_result_records.sort {|x,y| y.bookmark.user_count <=> x.bookmark.user_count}
          for result in result_records
            result_hash = Hash.new
            result_hash["url"] = result.bookmark.url
            result_hash["id"] = result.id
            result_hash["user"] = User.find(the_userid).name
            result_hash["title"] = result.title
            result_hash["description"] = result.description
            result_hash["copied_by"] = result.bookmark.user_count
            ## now retrieving tags for this user-bookmak mapping
            ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
            ## Append all these into a comma separated string, and push them onto the hash

            tag_fetchs = BmappingsTag.where(["bmapping_id = ?",result.id])
            tag_array = Array.new
            for tag_fetch in tag_fetchs
              tag_array << Tag.find(tag_fetch.tag_id).tagname
            end
            result_hash["tags"] =BookmarksHelper.join_tags(tag_array)
            result_array << result_hash
          end
        elsif (order_by == "most_popular" and !the_userid) ## Most popular tags for all users
        ## returns the url boomarked by the most number of users, the discoverer of that url, the title and description provided by the discoverer
        result_records = Bookmark.all(:order =>"user_count DESC", :limit =>20)
        for result in result_records
          ## for each tuple returned by the query above, create a new hash, store the values appropriately, and append into the return_array
          result_hash = Hash.new
          result_hash["url"] = result.url
          result_hash["user"] = User.find(result.discoverer_user_id).name
          result_hash["copied_by"] = result.user_count
          b_u_mapping = Bmapping.where(["bookmark_id = ? and user_id = ?", result.id, result.discoverer_user_id]).first
          result_hash["id"] = b_u_mapping.id
          result_hash["description"] = b_u_mapping.description
          result_hash["title"] = b_u_mapping.title
          ## now retrieving tags for this user-bookmak mapping
          ## first retrieve all the tag_ids mapped to the BMapping id. Then retrieve all the tag names of the tag_ids picked up.
          ## Append all these into a comma separated string, and push them onto the hash

          tag_fetchs = BmappingsTag.where(["bmapping_id = ?",b_u_mapping.id])
          tag_array = Array.new
          for tag_fetch in tag_fetchs
            tag_array << Tag.find(tag_fetch.tag_id).tagname
          end
          result_hash["tags"] = BookmarksHelper.join_tags(tag_array)
          result_array << result_hash
        end

        end

        return result_array
      end

Case 4 : Refactoring add_bmapping and add_bmapping_signuptopic methods

The methods ,add_bmapping and add_bmapping_signuptopic, add new bmapping to newly added bookmarks. These methods are initially served in bookmarks model. Ideally, they should be serving the bmapping model. add_bmapping_signuptopic is a continuation to add_bmapping. It adds signup topic to the newly created bmapping. Along with moving these methods from bookmarks model, the team had to check for dependencies for these methods in all corresponding models and take care of them.

Before Changes

Changes done to bmapping model. The new added code in this model has been moved from bookmarks model.

class Bmapping < ActiveRecord::Base
  belongs_to :user
  belongs_to :bookmark
  has_many :bmappings_tags
  has_and_belongs_to_many :sign_up_topics
  has_many :ratings, :class_name => "BmappingRatings", :foreign_key => "bmapping_id"

  def cumulative_rating
    rating = 0.0
    count = 0
    self.ratings.each do |br|
      rating = rating + br.rating
      count = count + 1
    end
    if count > 0
      return rating/count
    else
      return nil
    end
  end
end

After Changes

class Bmapping < ActiveRecord::Base
  belongs_to :user
  belongs_to :bookmark
  has_many :bmappings_tags
  has_and_belongs_to_many :sign_up_topics
  has_many :ratings, :class_name => "BmappingRatings", :foreign_key => "bmapping_id"

  def cumulative_rating
    rating = 0.0
    count = 0
    self.ratings.each do |br|
      rating = rating + br.rating
      count = count + 1
    end
    if count > 0
      return rating/count
    else
      return nil
    end
  end


  # Add bookmark - user association with its meta fields
    def self.add_bmapping(bid, b_title, user_id, b_description,b_tags_text)
      bookmark_user_mapping = Bmapping.new
      bookmark_user_mapping.bookmark_id = bid
      bookmark_user_mapping.title = b_title
      bookmark_user_mapping.description = b_description
      bookmark_user_mapping.user_id =user_id
      current_timestamp = Time.now
      bookmark_user_mapping.date_created = current_timestamp
      bookmark_user_mapping.date_modified = current_timestamp
      bookmark_user_mapping.save
      # Add tags
      # tags come in as a text, separating them into a array

      tag_array = BookmarksHelper.separate_tags(b_tags_text)
      for each_tag in tag_array
        # Look for each tag that is present in tags, if not make them, then make the BTU entry
        tag_tuple = Tag.where(["tagname = ?",each_tag]).first
        if !tag_tuple # enter this block if tag_tuple is nil
          tag_tuple = Tag.new
          tag_tuple.tagname = each_tag
          tag_tuple.save
        end
        # Check if there is an entry for this tag, this user and this bookmark (via bmappings table)
        btu_tuple = BmappingsTag.where([ "tag_id = ? and bmapping_id = ?", tag_tuple.id, bookmark_user_mapping.id] ).first
        if !btu_tuple # enter this block if btu_tuple is nil
          btu_tuple = BmappingsTag.new
          btu_tuple.tag_id = tag_tuple.id
          btu_tuple.bmapping_id = bookmark_user_mapping.id
          btu_tuple.save
        end
      end
        return bookmark_user_mapping.id
    end

      # Associate bmapping to the sign up topic
      def self.add_bmapping_signuptopic(topic_id, bmappingid)
        topic = SignUpTopic.find(topic_id)
        bmapping = Bmapping.find(bmappingid)
        unless (!topic && !bmapping)
          topic.bmappings << bmapping
          topic.save
        end
      end

end

Before Changes

The dependencies for these methods are present in only add_bookmark method of bookmark model. As add_bmapping and add_bmapping_signuptopic methods support add_bookmark in creating new bmapping to bookmarks,the dependencies lie only in add_bookmark method.

# Adds a bookmark and its various associations
    def self.add_bookmark(b_url, b_title, b_tags_text, b_description,session_user,topicid)
      bookmark_resource = Bookmark.where(["url = ?",b_url]).first

      # Bookmark with the same url does not exists.
      if !bookmark_resource
        # Add the newly discovered bookmark
        bookmarkid = add_new_bookmark(b_url,session_user.id)
        # Add its associations to a user
        bmappingid = add_bmapping(bookmarkid, b_title, session_user.id, b_description,b_tags_text )
        # Add its association to the sign up topic
        add_bmapping_signuptopic(topicid, bmappingid)

        # Bookmark with the same url exists.
      else
        bmapping = Bmapping.where(bookmark_id: bookmark_resource.id, user_id: session_user.id).first
        # Bookmark with the same user - bmapping exists.
        if (bmapping)
          topic = SignUpTopic.find(topicid)
          if(topic)
            topic.bmappings.each do |mapping|
              if (mapping.id == bmapping.id)
                Bookmark.edit_this_bookmark(b_url, b_title, b_tags_text, b_description,session_user)
              end
            end

            # Signup Topic does not exists
          else
            add_bmapping_signuptopic(topicid, bmapping.id)
          end

          # Bookmark with same user - bmapping does not exists.
        else
          # Increment user count for the existing bookmark
          bookmark_resource.user_count = bookmark_resource.user_count + 1
          bookmark_resource.save
          # Add its association with the user
          bmappingid = add_bmapping(bookmark_resource.id, b_title, session_user.id, b_description,b_tags_text)
          add_bmapping_signuptopic(topicid, bmappingid)
        end
      end
    end

After Changes

# Adds a bookmark and its various associations
  def self.add_bookmark(b_url, b_title, b_tags_text, b_description,session_user,topic_id)
    bookmark_resource = Bookmark.where(["url = ?
      ",b_url]).first

    # Bookmark with the same url does not exists.
    if !bookmark_resource
      # Add the newly discovered bookmark
      bookmarkid = create(b_url,session_user.id)
      # Add its associations to a user
      bmappingid = Bmapping.add_bmapping(bookmarkid, b_title, session_user.id, b_description,b_tags_text )
      if(topic_id)
        # Add its association to the sign up topic if the topic was provided
        Bmapping.add_bmapping_signuptopic(topic_id, bmappingid)
      end
      # Bookmark with the same url exists.
    else
      bmapping = Bmapping.where(bookmark_id: bookmark_resource.id, user_id: session_user.id).first
      # Bookmark with the same user - bmapping exists.
      if (bmapping)
        topic = SignUpTopic.find(topic_id)
        if(topic)
          topic.bmappings.each do |mapping|
            if (mapping.id == bmapping.id)
              Bookmark.edit(b_url, b_title, b_tags_text, b_description,session_user)
            end
          end

          # Signup Topic does not exists
        else
          Bmapping.add_bmapping_signuptopic(topic_id, bmapping.id)
        end

        # Bookmark with same user - bmapping does not exists.
      else
        # Increment user count for the existing bookmark
        bookmark_resource.user_count = bookmark_resource.user_count + 1
        bookmark_resource.save
        # Add its association with the user
        bmappingid = Bmapping.add_bmapping(bookmark_resource.id, b_title, session_user.id, b_description,b_tags_text)
        Bmapping.add_bmapping_signuptopic(topic_id, bmappingid)
      end
    end
  end

Steps to verify changes manually

User2 Role

In order to inspect the changes made to the bookmark mode manually, you can visit our deployed Expertiza at Demo and use the following credentials:

    User Name:  user2
    Password:   any password

As you can notice in the credentials above, you can type in any password you wish in order to log in as user2. We have accomplished this by disabling the authentication and the reason behind it was to have an easily reachable user account for test and development purposes.

The below screenshots demonstrate the working of search features .

Search for all tags for all users

Visit the url " http://localhost:3000/bookmarks/viewing_bookmarks" and click on search button to view the results

Search for all tags for user

Visit the url " http://localhost:3000/bookmarks/managing_bookmarks " to view all tags for the user

Search for tags for all users

Visit the url " http://localhost:3000/bookmarks/view_bookmarks " , enter the tags separated by comma (example - tag1,tag2) and then click on search button to view the results

Search for tags for user

Visit the url "http://localhost:3000/bookmarks/managing_bookmarks" , enter the tags separated by comma (example - tag1,tag2) and then click on search button to view the results

Automated Tests

Functional test suite

We have developed a suit of extensive unit tests covering all the functionality that we have add/modified in the forked version of Expertiza. The test suite consists of 9 tests providing 14 assertions that cover the functionality of all the method changes described in the former sections of this wiki page. The source code for these tests is stored in test/models/bookmark_test.rb file. You can run this test suite by simply navigating to expertiza project directory and running command:

ruby -I test test/models/bookmark_test.rb

Integration (Cucumber) tests

In addition to functional tests, we have also provided a Cucumber test as an integration test. The goal of this test is to show that a user can access the Manage Bookmarks page upon successful log in, proving that bookmarks view have been successfully integrated into expertiza system. The construct of this test can be found in following files:

features/bookmark.feature 
features/step_definitions/bookmark_steps

To run this cucumber test, please navigate to expertiza project directory and run the following command:

rake cucumber

References

1. Expertiza on GitHub
2. GitHub Project Repository Fork
3. The live Expertiza website
4. Demo link - This might not be available after May of 2015
5. Expertiza project documentation wiki
6. The OSS project requirements doc (Spring 2015 - Expertiza)
7. Demo of Test suite
8. Single Responsibility Principle