lml / ost

OpenStax Tutor
Other
7 stars 8 forks source link

lml/ost#341: Add aggregated assignment exercise view. #344

Closed navilan closed 10 years ago

navilan commented 10 years ago

WIP.

Notes:

jpslav commented 10 years ago

@lakshmivyas, the WIP looks good. Agree that we need to handle multiple cohorts. Re redundancy in the views, agree on that too. I'd say we can refactor those partials/views now if isn't a crazy big change. Otherwise we can punt and revisit later after the UX team makes a pass at critical pages.

Let me and @kjdav know when multiple cohorts are addressed and we can revisit / test. thx!

navilan commented 10 years ago

@kjdav: Thanks. I made the multiple choices collapsed. So the spacing might not be a big issue. Have a look and please let me know.

navilan commented 10 years ago

@jpslav : 1) Refactoring: I think its better to wait until UX review. It also gives me time to understand the different pieces so that I can refactor more effectively. 2) I will get the multiple cohorts done tomorrow. I had some difficulty setting up students under multiple cohorts. (A nicely setup dev DB sounds very useful suddenly ;)) 3) Research vs Educators: Yes, @kjdav explained it nicely during our demo. You can see that I have removed the link to assignment_exercises for researches for now. The choices are: a) Show the page but hide the non-consenting students b) Keep the page just for educators. I can reinstate the link for researchers once we discuss this.

jpslav commented 10 years ago

Choice 3(a) sounds good.

On a related note, hiding the link in cases like this is definitely part of the solution, but we also need to make sure access to the page is restricted in and of itself (the controller action for that page should check that the user can visit it). Just hiding the link would still let someone manually type in the URL for what should be a restricted page.

kjdav commented 10 years ago

1) I like the way the separators look.

2) How are students sorted in the new view? in my db, i see students listed in alphabetical order on the main page. but in the new view, they are no longer alphabetical. they are also no longer separated by registered/auditing/dropped.

3) In the new view, we should have breadcrumbs back to the main assignment page and back to the class page.

4) this is more of a ux decision but it'd be good to have navigation links in the new view, so instructors can a) navigate to "next question" and "previous question" and also b) to "view next cohort"

5) I like the way you collapsed the multiple choices. Minor but should the Choices header be flush with Exercise instead of with the question itself? I guess I could see either way. Also instead of "Choices" I think prefer "Answer Choices."

jpslav commented 10 years ago

Good suggestions @kjdav !

navilan commented 10 years ago

@kjdav : Thanks so much. Will get these done next. @jpslav : Regarding page level restriction, currently a security transgression is thrown for the researcher as the assignment_exercise model does not provide access for them. All of this is moot once we implement 3(a) above. However, as a general question, how is security transgression handled on the UI? Is there a middleware (or something similar) that can catch and display a special page / content?

As with the other PR's I am closing this one in favor of a new consolidated one.

jpslav commented 10 years ago

Exceptions and errors are caught in the controller base class (the ApplicationController). A generic 403 page is returned. Ideally, there shouldn't be links to lead a user to cases where this would happen. If someone isn't logged in, before_filter :authenticate_user! will redirect them to the login page before they get close to where the permissions are checked.

navilan commented 10 years ago

I see - makes sense. I guess the production environment will show a different, locked-down page when an exception occurs? I will try this out sometime later this week.