Feature/exercises #348
Feature/exercises #348
Conversation
This commit reintroduces lml#342. - Add rbenv.md that documents installation procudure for OST with rbenv. - Remove section about capybara-webkit from the document. - Add vendor/cache to gitignore. - Add ruby version file to the repository.
- Create a partial layout for section containers. - Create paritals for question, choices and correct answer under exercise. - Create a blurb partial for exercise. - Set permissions for assignment exercise (educator || researcher). - Add links to student assignment to get to the assignment exercises page. - Add the assignment exercises controller and view.
- Reuse exercise blurb from assignment_exercise. - Remove redundant `if true` statements. - Add boolean flags for showing correct answer and choices. - Use defined check to provide defaults for boolean values.
- Ensure that multiple choices are laid out in columnar fashion. - Add mark_correct variable to toggle correct answer tagging in the HTML element with a class. This is needed to make sure correct answer is not revealed through view source :)
- Add link to the class. - Add link to the assignment. - Add a helper class for getting previous and next assignments. - Add prev / next links in nav.
- Sort students by their status (registered, auditing, dropped). - Use the sorted list to organize the responses. - Create a partial to render the reponse list per group.
| # Copyright 2011-2014 Rice University. Licensed under the Affero General Public | ||
| # License version 3 or later. See the COPYRIGHT file for details. | ||
|
|
||
| module AssignmentExerciseHelper |
There was a problem hiding this comment.
Normally, helpers are used to get logic out of a view. In this case since we're dealing purely with model logic, I think we could safely move this code into the AssignmentExercise model by adding a prev and a next method to it.
Since AssignmentExercises are numbered, we can also avoid querying for the entire array of assignment exercises in an exercise with something like this untested code:
def next
AssignmentExercise.where{number == my{number}+1}.where{assignment_id == my{assignment_id}}
end
Save us a query and the query we run only has to return one record instead of all AEs for an assignment.
Note that the curly bracket where syntax and the my call come from a gem called squeel which gives some niceties on top of ActiveRecord.
If you didn't want to duplicate the .where{assignment_id == my{assignment_id}} code between the prev and next calls, you could introduce a scope for it:
scope :peers, where{assignment_id == my{assignment_id}
def next
AssignmentExercise.peers.where{number == my{number}+1}
end
And now that I've written all of this, I have realized that these prev and next methods should be added to lib/acts_as_numberable.rb, which is the library that manages the numbered nature of this model.
If you look at the top of app/models/assignment_exercise.rb you'll see the call that makes a bunch of number-related things work in this class:
acts_as_numberable :container => :assignment
Since the acts_as_numberable library already knows the container object (as well as the number field), it can just implement prev and next on its own (and that way they'll be available to all numbered models.
Maybe take a look at the lib file -- if it makes sense where you'd put the code, go ahead and do it. If not immediately clear, we can chat after scrum tomorrow.
There was a problem hiding this comment.
Wow - excellent. Thanks so much JP. Great pointers there. acts_as_numberable looks spiffy. Will do this today.
- Remove the assignment_exercise helper object. - Add peers scope to container instance. - Add prev / next method to container instance. - Use prev / next in the sidebar nav.
- Use tips from the most excellent @Dantemss. - Add a scope `by_student` to student_exercises along with requisite relations. - Use an app level group by to avoid sending out 3 queries.
|
@Dantemss: The last commit has the new query. There is a harmless but nevertheless annoying issue where joins are repeated. Other than that seems to be a good solution. Thanks so much. @jpslav : I added prev/next methods to acts as numberable - please let me know if it looks good. @kjdav : Kim, at this point, I think that functionally the page is complete - whats remaining from my rather limited perspective is polish (styling, testing, optimization). If you can look through it one more time and let me know your comments, I can address them as a final pass before this gets merged. |
| # Get assignments for the students | ||
| exercises = student_exercises.by_student(present_user) | ||
|
|
||
| exercises.group_by { |ex| case |
There was a problem hiding this comment.
Just a minor thing, but brackets are usually used for single-line blocks. For multiline blocks, we usually use the "do end" syntax.
There was a problem hiding this comment.
Ah, yes. Forgot about that. Will fix and remember.
There was a problem hiding this comment.
Of course, sometimes Ruby just won't accept the do...end syntax (I think when defining lambdas, for example). In that case, brackets are fine.
- Use inline join to the user table to get rid of the redundant joins introduced by rails' merge method. - present_user => user.
- Added comments for weird code bits. - Changed scope name from by_student to visible_by_student. - Changed to do/end from braces. - Changed present_user => user.
- The peers scope was incorrectly using an assignment instead of a query. Used a method instead of scope. - Corrected typo "availble" - Changed navigation items from "Go to x" to "x" - Removed the Exercise link from feedback nav. - Reordered class and assignment in AE nav as in other pages. - "Your free responses" title cased.
| if user.is_researcher? || user.is_visitor? | ||
| # The dummy "1 = 1" condition is added in order to use the | ||
| # merge method. | ||
| visible = where("1 = 1").merge(Student.visible(user)) |
There was a problem hiding this comment.
Just replace merge with a subquery using IN and you don't need all this handcoded SQL.
There was a problem hiding this comment.
@Dantemss - I need to order the resulting student exercises by the student last name and student status - so, having the students in the subquery won't be sufficient. Does that make sense?
There was a problem hiding this comment.
Please avoid any hand-coded SQL. Even if we need to break the query up into steps, I'd rather keep the queries only using the abstracted query syntax provided by Rails.
There was a problem hiding this comment.
This is the query I get from the above:
SELECT
"student_exercises".*
FROM "student_exercises"
INNER JOIN
"student_assignments" ON "student_assignments"."id" = "student_exercises"."student_assignment_id"
INNER JOIN
"students" ON "students"."id" = "student_assignments"."student_id"
INNER JOIN "users" ON "students"."user_id" = "users"."id"
WHERE "student_exercises"."assignment_exercise_id" = 10
ORDER BY "students"."has_dropped", "students"."is_auditing", lower("users"."last_name")This is exactly the query that's needed. I can understand the avoid handcoded sql argument. But don't you guys think in this scenario (where rails is unable to produce the optimal sql output) - judicious sprinkling of handcoded sql makes for a performant page?
There was a problem hiding this comment.
Here is the one for researchers:
SELECT
"student_exercises".*
FROM "student_exercises"
INNER JOIN
"student_assignments" ON "student_assignments"."id" = "student_exercises"."student_assignment_id"
INNER JOIN
"students" ON "students"."id" = "student_assignments"."student_id"
INNER JOIN
"consents" ON "consents"."consentable_id" = "students"."id"
AND "consents"."consentable_type" = 'Student'
INNER JOIN "users" ON "students"."user_id" = "users"."id"
WHERE "student_exercises"."assignment_exercise_id" = 10
AND "consents"."did_consent" = 't'
AND (1 = 1)
ORDER BY "students"."has_dropped", "students"."is_auditing", lower("users"."last_name")There was a problem hiding this comment.
You mean making a new gem? Probably not for now.
There was a problem hiding this comment.
Something in the lib dir (like ActsAsNumberable). But yeah - there are just two occurrences, I guess we can wait until there is one more.
There was a problem hiding this comment.
Yeah I agree we should hold off on library-ifying the consenting infrastructure. The only user at this point is Tutor. Maybe when we redo Tutor it'll make sense to abstract the consent stuff into a gem.
There was a problem hiding this comment.
There's a whole mess of consenting infrastructure around, so a gem is the right idea just don't know if we're at the point where we want to spend time consolidating and generalizing it.
I owe @Dantemss several beers and my sanity. - Separate scopes for visible and by_student. - Use fully qualified objects to avoid duplicate joins. - Remove hardcoded query.
|
this looks good from my perspective, both the changes to the branch and On Thu, May 22, 2014 at 4:15 PM, Lakshmi [email protected] wrote:
|
- Add support for prev/next in BasicInstance. - Use sifters to encapsulate column names. - Add a peers method to the BasicInstance that is essentially just an identity method.
| @@ -9,7 +9,6 @@ | |||
|
|
|||
| <%= link_to 'Show Class', student_exercise.klass %><br/> | |||
| <%= link_to 'Show Assignment', student_exercise.assignment %><br/> | |||
| <%= link_to 'Show Exercise', student_exercise %><br/> | |||
There was a problem hiding this comment.
Our thought was this link is no longer needed, since the feedback page now shows the exercise.
There was a problem hiding this comment.
Do you mean put on the same page and have a link that populates feedback via ajax?
There was a problem hiding this comment.
Yeah - combine the feedback and the exercise page. I don't think ajax is needed though - unless I am missing something. Can't we just hide the element and show it only on click. When the link/button is clicked, we make an API call to set the "feedback viewed" status.
But the feedback content can be pre-rendered. Do you see any problems with this approach?
There was a problem hiding this comment.
Can't put it in the page because students will cheat (they will find it and just read the answer)
There was a problem hiding this comment.
Only after evaluation:
<% if evaluated %>
show feedback link
<% endif %>
Essentially, since the feedback link is only shown (?) after the student completes the
exercise and the feedback rules allow, we will prerender the feedback content on the
exercise page based on those same criteria.
There was a problem hiding this comment.
oh ok if you mean we do a full page refresh and just render the feedback content when the exercise is complete, that's probably fine. these are good thoughts that hopefully the UX folks can build on when we do the next tutor iteration.
There was a problem hiding this comment.
Ah - now I understand what you meant by needing ajax - realtime feedback. Yeah makes sense.
Good, the first collab item on basecamp will be a collection of these UXy things ;)
|
Thanks @lakshmivyas! |
WIP. Refactored and consolidated the previous pull requests.
collapsabilityparams (solved with a partial layout instead.)Fix layout and styles to make the page look and feel better.(Deferred until later).Figure out if tests can and need to be added for complex bits.(Saving these for later)