Merge branch 'remove-redirect' into 'master'

Remove redirect

See merge request !39

schedules/.rubocop.yml

@@ -16,3 +16,6 @@ Metrics/BlockLength:
 
 Style/ClassAndModuleChildren:
   Enabled: false
+
+Style/GuardClause:
+  Enabled: false

schedules/app/controllers/application_controller.rb

 # Configures the application.
 class ApplicationController < ActionController::Base
   protect_from_forgery with: :null_session
-  before_action :set_semester, :set_cookies, :set_cart
 
+  # On each request, set the semester and cart.
+  before_action :set_semester, :set_cart
+
+  # Every page needs to know what semester it should load data from.
+  # set_semester checks both the semester_id query parameter and the user's cookies
+  # to look for a semester id and loads whatever it finds into @semester.
+  #
+  # By default, load the most recent semester.
   def set_semester
     if params.key?(:semester_id)
-      cookies[:semester_id] = params[:semester_id]
       @semester = Semester.find_by_id params[:semester_id]
+      cookies[:semester_id] = @semester.id
     elsif cookies[:semester_id].nil?
-      redirect_to(url_for(params.permit(params.keys).merge(semester_id: Semester.first.id)))
+      @semester = Semester.first
+      cookies[:semester_id] = @semester.id
     else
-      redirect_to(url_for(params.permit(params.keys).merge(semester_id: cookies[:semester_id])))
+      @semester = Semester.find_by_id cookies[:semester_id]
     end
   end
 
+  # The user's cart is stored as a JSON-encoded list of CRNs.
+  # set_cart sets the @cart variable, which is a list of the sections represented by the CRNs.
   def set_cart
+    # set the cart cookie to be empty if it doesn't already exist
+    cookies.permanent[:cart] = "[]" if cookies.permanent[:cart].nil?
+
+    # decode the JSON list into an array
     @cart = JSON.parse(cookies.permanent[:cart])
+
+    # get rid of any invalid CRNs
     @cart = @cart.reject { |crn| CourseSection.find_by_crn(crn).nil? }
-    cookies.permanent[:cart] = @cart.to_json
-  end
 
-  def set_cookies
-    cookies.permanent[:cart] = "[]" if cookies.permanent[:cart].nil?
+    # set the cookie to the JSON-encoded list of valid sections
+    cookies.permanent[:cart] = @cart.to_json
   end
 end

schedules/app/controllers/courses_controller.rb

 class CoursesController < ApplicationController
-  before_action :set_course
-
   def show
-    @course = Course.find_by subject: @course.subject, course_number: @course.course_number, semester: @semester
-  end
+    # Load the course with the id passed in the URL.
+    @course = Course.find_by_id(params[:id])
+
+    # If the user changes the semester while looking at a course,
+    # we need to display the page for the course with the same subject and course number
+    # in that semester.
+    unless @course.semester == @semester
+      # find the course we should redirect to
+      @course = Course.find_by(subject: @course.subject, course_number: @course.course_number, semester: @semester)
 
-  private
+      # redirect to the url for that course
+      redirect_to course_url(@course)
+    end
 
-  def set_course
-    @course = Course.find_by_id params[:id]
+    @sections = @course.course_sections
   end
 end

schedules/app/controllers/home_controller.rb

+# HomeController renders the homepage. It currently requires no logic.
 class HomeController < ApplicationController
   def index; end
 end

schedules/app/controllers/instructors_controller.rb

 class InstructorsController < ApplicationController
-  before_action :set_instructor, only: [:show]
-
   def index
     @instructors = Instructor.all
   end
 
   def show
-    sections = CourseSection.where instructor: @instructor
-    sections = sections.select do |s|
-      s.course.semester == @semester
-    end
+    @instructor = Instructor.find_by_id(params[:id])
 
-    # TODO: move this to a model somewhere
-    @courses = [].to_set
-    sections.each do |s|
-      @courses.add s.course
-    end
+    # find the courses being taught this semester
+    sections = CourseSection.where(instructor: @instructor).joins(course: :semester).where("semesters.id = ?", @semester.id)
+    @courses = Course.build_set(sections)
 
+    # build the list of courses the instructor has taught in the past
     @past = []
     @instructor.course_sections.map(&:course).each do |c|
       @past << c unless @past.select { |past| past.full_name == c.full_name }.count.positive?
     end
     @past.sort_by!(&:full_name)
   end
-
-  private
-
-  def set_instructor
-    @instructor = Instructor.find_by_id params[:id]
-  end
 end

schedules/app/controllers/search_controller.rb

 class SearchController < ApplicationController
   def index
-    redirect_to home_url unless params[:query].length > 1
+    redirect_to(home_url) unless params[:query].length > 1
 
     results = SearchHelper::GenericItem.fetchall(String.new(params[:query]), semester: @semester).group_by(&:type)
     @instructors = results[:instructor]&.map(&:data)

schedules/app/controllers/sessions_controller.rb

 class SessionsController < ApplicationController
-  def update
-    update_cookie :crns
-    update_cookie :section_ids
-    update_cookie :semester_id
-
-    head :ok
-  end
-
   def cart
     section_crn = params[:crn]
 
@@ -16,25 +8,18 @@ class SessionsController < ApplicationController
       @cart << section_crn
     end
 
-    puts @cart
     cookies.permanent[:cart] = @cart.to_json
     render json: @cart.to_json
   end
 
   def add_bulk
     crns = params[:crns].split(',')
-    crns.each { |crn|
+    crns.each do |crn|
       s = CourseSection.latest_by_crn(crn)
       next if s.nil?
       @cart << crn.to_s unless @cart.include?(crn.to_s)
-    }
+    end
     cookies.permanent[:cart] = @cart.to_json
-    redirect_to schedule_path
-  end
-
-  private
-
-  def update_cookie(sym)
-    cookies[sym] = params[sym] unless params[sym].nil?
+    redirect_to(schedule_path)
   end
 end

schedules/app/models/course.rb

@@ -52,4 +52,13 @@ class Course < ApplicationRecord
 
     query
   end
+
+  # build_set builds
+  def self.build_set(sections)
+    courses = [].to_set
+    sections.each do |s|
+      courses.add s.course
+    end
+    courses
+  end
 end

schedules/app/views/courses/show.html.erb

@@ -16,7 +16,7 @@
 		    <div class="icon">
 			<i class="fa fa-bars"></i>
 		    </div>
-		    <%= @course.course_sections.count %> sections
+		    <%= @sections.count %> sections
 		</div>
 	    </div>
 	</div>
@@ -24,7 +24,7 @@
     </div>
 
     <div class="col-12 col-lg">
-	<%= render partial: 'shared/section', collection: @course.course_sections %>
+	<%= render partial: 'shared/section', collection: @sections %>
     </div>
 </div>
 

schedules/app/views/instructors/show.html.erb

@@ -5,7 +5,7 @@
 	    <strong>Previously taught: </strong>
 	    <ul>
 		<% @past.each do |c| %>
-		    <li><%= link_to c.full_name, course_path(c)  %></li>
+		    <li><%= link_to(c.full_name, course_path(c)) %></li>
 		<% end %>
 	    </ul>
 	<% end %>
@@ -14,7 +14,7 @@
     <div class ="col-lg-8 col-12">
 	<h3><%= @semester.to_s %></h3>
 	<% if @courses.any? %>
-	    <%= render partial: 'shared/course', collection: @courses, locals: { expanded: true } %>
+	    <%= render(partial: 'shared/course', collection: @courses, locals: { expanded: true }) %>
 	<% else %>
 	    <p><%= @instructor.name %> is not teaching any courses this semester...</p>
 	<% end %>

schedules/test/controllers/application_controller_test.rb

 require 'test_helper'
 
 class ApplicationControllerTest < ActionDispatch::IntegrationTest
-  test "redirects without semester id" do
-    get "/"
-    assert_response :redirect
-    assert_match /\?semester_id=#{semesters(:fall2018).id}/, @response.redirect_url
+  test "gets home page" do
+    get("/")
+    assert_response(:success)
   end
 end