Student task status and study plan /wip #23

Open
wants to merge 46 commits into
from

Projects

None yet

3 participants

@seejee

Hi everyone,

@wicz and I are still working on this, but we'd like some preliminary feedback.

We've got tasks, their statuses, and a link to the study plan being displayed the student page. The course page provides a link to the student page.

In the next day or two, we're going to finish up marking tasks as complete from the UI, and showing/editing study plans.

Feedback is welcome. Thanks!

wicz and others added some commits Mar 22, 2012
@wicz wicz Merge branch 'master' of github.com:mendicant-university/liskov
* 'master' of github.com:mendicant-university/liskov:
  Enable task removal
  Add database cleaners and move specs into their own support file
  Simplify discussion link generation
  highlight category
  add Discussion.search method and refactor DiscussionController
  Add a discussion list decorator
  horribly rough toggling of open/closed status
  hide archived discussions from main view
f077011
@seejee seejee just started with student page e1b2dcd
@seejee seejee forgot to add some stuff 6ade83a
@wicz wicz Add link to students page 48ce60c
@wicz wicz Resolve conflicts c4620b5
@seejee seejee fixed test broken by new task factories c956ccb
@seejee seejee DRY'd up how PersonDecorator are created and added filter chain in st…
…udents controller
4c036a1
@seejee seejee naive passing student page test fcdbeb8
@wicz wicz Merge branch 'master' of github.com:wicz/liskov
* 'master' of github.com:wicz/liskov:
  naive passing student page test
  DRY'd up how PersonDecorator are created and added filter chain in students controller
  fixed test broken by new task factories
852bbdb
@wicz wicz Add distinct links for students and instructors ee5689c
@seejee seejee refactored student page integration spec to use data attribs 2a45108
@seejee seejee showing course task statuses b0a8abc
@seejee seejee added constant to represent task is not completed 059448f
@seejee seejee added failing spec for instructors seeing a task completion link 9cb0545
@seejee seejee using correct assertion in student page test f388442
@seejee seejee showing link to update a task only for instructors 5f2eaf9
@seejee seejee much better use of decorators f22370d
@seejee seejee link text should not show for instructors 7d70cdc
@seejee seejee little bit of cleanup before being able to complete course tasks f2fec3a
@seejee seejee added model logic to marks tasks as complete 116709b
@seejee seejee ensure completing the same task twice does not create a new completed…
… task record
491784d
@wicz wicz DRY find_course to ApplicationCtrlr cf3eb13
@wicz wicz Add StudyPlan model 3bd977e
@wicz wicz Adds single method to read from Clubhouse. Checks for Liskov permissions e129ef6
@wicz wicz Always raise 404 when resource not found 08865ca
@wicz wicz helper_method only what needed in views cf315ca
@wicz wicz DRY find_student for student-dependent ctrlrs bc0188a
@wicz wicz Refactoring and updating tests 183c29d
@wicz wicz Add study plan /wip 48e52e5
@seejee seejee merged from wicz/study-plans b534eac
@seejee seejee merging with mendicant-university/liskov master c5ac30a
@jordanbyron jordanbyron and 2 others commented on an outdated diff Mar 27, 2012
app/controllers/application_controller.rb
@@ -34,10 +29,27 @@ def login_path
Rails.env.production? ? '/auth/github' : '/auth/developer'
end
- private
+ protected
@jordanbyron
jordanbyron Mar 27, 2012

Why the change from private to protected?

@wicz
wicz Mar 27, 2012

ops, looks the former java programmer inside me is still alive. 😉

@jordanbyron
mendicant-original member

@geihsler & @wicz we need to talk about these tests. I'm really glad you are both writing tests, but a bunch of these are going to be thrown out once we restructure and finalize the views. That means I'll have failing tests anytime I make a minor change which isn't good.

What I think I'll do is after we merge this in, I'll create another pull request removing the unnecessary tests. Then in that pull request we can discuss why I removed them.

@jordanbyron jordanbyron and 1 other commented on an outdated diff Mar 27, 2012
app/decorators/task_decorator.rb
@@ -0,0 +1,41 @@
+class TaskDecorator < ApplicationDecorator
@jordanbyron
jordanbyron Mar 27, 2012

This decorator is a bit confusing, because I'd expect the TaskDecorator to decorate plain old Task objects, but in this case it doesn't. Instead it wraps a Task object and StudentDecorator object, which in the class is called a participant. This is what I'd call a StudentTask and isn't really a decorator. I think creating a proper StudentTask class is the right way to go here. That class should be able to build all the tasks for a student, much like StudentDecorator#tasks does. It should also handle returning the status of the task and contain the code from CourseMembership that has to do with tasks. How does that sound? It should help consolidate all the student task code which is spread across 2 or 3 classes.

@seejee
seejee Mar 27, 2012

@jordanbyron good feedback.

The participant name comes from the fact that we were toying with renaming CourseMembership to Participant but decided not to. I'll rename that.

The StudentTask idea is an interesting one that, as you say, might consolidate the code from 2 or 3 classes. I see this class as a non-persisted model. Do you agree? Also, I think we'd then need a StudentTaskDecorator because #status and #complete_task_link are presentation logic.

I'll play with this in our repo and ask you to review a pull request.

@seejee
seejee Mar 27, 2012

@jordanbyron Please take a look at this pull request where I made some of the changes that you recommended. Overall, I like it. What do you think?

@seejee

@jordanbyron I'd like to talk about the tests as well. They've been very useful when developing these features, especially the integration tests. I'm curious to see which ones you think add to much friction to change. If a test is limiting change, than I'd like to find a way to make it a little more tolerant to change instead of deleting it. That being said, I'm not at all opposed to deleting tests that provide little or no value.

@jordanbyron jordanbyron commented on an outdated diff Mar 28, 2012
app/models/student_task.rb
+
+ def valid?
+ @course_membership.valid?
+ end
+
+ def complete?
+ status != StudentTask::NOT_COMPLETE
+ end
+
+ def status
+ completed = get_completed_task
+ completed ? completed.description : NOT_COMPLETE
+ end
+
+ def complete(status)
+ #TODO: is there a better way to do an upsert? - cg
@jordanbyron
jordanbyron Mar 28, 2012

I think you're looking for:

course_membership.completed_tasks.find_or_create_by_task_id(:task_id => task_id, :description => status)
@jordanbyron jordanbyron commented on an outdated diff Mar 28, 2012
app/models/student_task.rb
+ end
+
+ def complete(status)
+ #TODO: is there a better way to do an upsert? - cg
+ completed = get_completed_task || completed_tasks.build(task_id: task_id)
+ completed.description = status
+ completed.save
+ end
+
+ private
+
+ def completed_tasks
+ @course_membership.completed_tasks
+ end
+
+ def get_completed_task
@jordanbyron
jordanbyron Mar 28, 2012

Let's rename this to just completed_task. The get part is implied and reminds me of java / C# 😳

@jordanbyron jordanbyron commented on an outdated diff Mar 28, 2012
app/views/student_tasks/edit.html.haml
@@ -0,0 +1,7 @@
+%p
+ =@student_task.description
+
+=form_tag @student_task.update_url, method: :put do
+ =label :student_task_, :status
+ =text_field :student_task, :status
+ =submit_tag "Submit"
@jordanbyron
jordanbyron Mar 28, 2012

This is a minor nit-pick, but add a space between the = operator and your ruby code. For example:

= form_tag do
  = lable_tag "Blah"
@jordanbyron jordanbyron and 1 other commented on an outdated diff Mar 28, 2012
app/decorators/student_decorator.rb
@@ -0,0 +1,25 @@
+class StudentDecorator < ApplicationDecorator
+ decorates :course_membership
+
+ def name
+ course_membership.person.name
+ end
+
+ def tasks
+ course_membership.student_tasks.map {|st| StudentTaskDecorator.new(st)}
+ end
+
+ def study_plan
+ course_membership.study_plan || course_membership.create_study_plan
+ end
@jordanbyron
jordanbyron Mar 28, 2012

This is another place where find_or_create_by can be used

@seejee
seejee Mar 28, 2012

study_plan is a :has_one on CourseMembership. I looked through the API docs, and I couldn't find any way to find_or_create on a :has_one or would it be something like StudyPlan.find_or_create_by_course_membership(course_membership)?

@seejee
seejee Mar 28, 2012

Nevermind. I got it.

@jordanbyron
mendicant-original member

This is coming along great guys. I really like the new StudentTask class. I made a few very small notes and once those are taken care of I think this is ready to merge. If either of you have any more questions just let me know. Great work! 🚀

@seejee

Thanks for the review @jordanbyron! I believe I've addressed all of the concerns you had.

StudentTask really cleaned things up. Great suggestion!

@wicz

Hey guys, I'd like to publicly thank @geihsler for all this work on this. You rock mate! 👏

I'd like also to apologize for my absence. The last couple of days was like hell in work and with personal problems, so I couldn't help with anything. I hope tonight I'll be able to catch up with you and work on the remaining tasks.

wicz added some commits Mar 29, 2012
@wicz wicz Merge branch 'master' of github.com:wicz/liskov
* 'master' of github.com:wicz/liskov: (32 commits)
  using find_or_create for StudyPlan
  one more haml style fix
  using find_or_create in StudentTask now
  renamed get_completed_task to completed_task
  fixing spaces after = in haml
  renamed CompletedTasksController to StudentTasksController
  marking tasks as complete works
  moved NOT_COMPLETE const to StudentTask and renamed spec
  moved student task logic into StudentTask model
  changing protected to private
  Add study plan /wip
  Refactoring and updating tests
  DRY find_student for student-dependent ctrlrs
  helper_method only what needed in views
  Always raise 404 when resource not found
  Adds single method to read from Clubhouse. Checks for Liskov permissions
  Add PersonDecorator.from_github_name shortcut
  add simple gravatar functionality
  Remove superfluous performance test
  Add StudyPlan model
  ...
7f2136b
@wicz wicz Parse study plan Markdown de433f9
@wicz wicz Update study plan 420c242
@wicz wicz Add md_preview dependency
Using my repo while the original can't work with rails 3.2
d1b7173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment