Puts course listings in a sane order

[pmitros]

@cpennington @nedbat Right now, if I go to edx.org, and look at the courses I'm taking, I have to scroll through 6 pages of things I signed up for but never touched. This is a supremely bad user experience. I'm sure I did a ton of things wrong, but this code applies a heuristic where courses are ordered on the page in roughly the order in which I last visited them (defined as "clicked around in courseware a little bit"). Recent courses are on top. Old courses are on bottom. I can go to courses I'm active in quickly and immediately.

[cpennington]

Imports at the top of the file, please.

[cpennington]

@shnayder This seems pretty useful. However, I could see it maybe being confusing to have the order of courses suddenly shuffled globally. I could also see a more general fix that allowed multiple sorting orders of the courses (and let the user pick between them).

[pmitros]

@cpennington Right now, they come out in whatever order the database happens to give them in. I'm not sure how it's structured, but we constantly add new courses, and students sign up for new ones, so I do not believe we have any sort of anti-shuffling guarantee ;)

[shnayder]

There is certainly more we want to do eventually, but this definitely seems like an improvement. Discussing with the design team.

In the meantime, @cpennington: any performance concerns from adding that query to CWSM?

[cpennington]

I think it would be good to make sure that this filters by both module_state_key and course_id. Do you see any reason not to do that?

[marcotuts]

@pmitros this is great! One suggestion would be to add a small text label right-aligned with the CURRENT COURSES header reading "ordered by recent activity".
Here is a quick 30 second wire for what that might look like - https://www.dropbox.com/s/3tacef2ly3qjhcf/Screenshot%202014-05-15%2015.23.15.png

The goal for this would be to eliminate potential confusion about reordering.

[jtauber]

@cpennington the order is so random-seeming at the moment, I doubt any students would find a change to a logical ordering confusing although I like @marcotuts's suggestion to explicitly indicate the sorting.

My dashboard currently has in-progress, completed and up-coming courses all interleaved. Even just separating these (with no specific ordering within the categories) would be a huge improvement.

One last comment: the heading "Current Courses" seems odd given it includes both completed and up-coming courses in addition to what are truly the "current" courses.

[jtauber]

I just watched the interviews from the 2nd birthday party and this request (to separate in-progress, completed and up-coming courses) was actually made by a student who tweeted :-)

[marcotuts]

@jtauber - It is worth noting that this PR doesn't group by those categories, but I agree this would be a useful extension of this page's layout. Regardless I think the sort order is a helpful addition to this page prior to working out how to handle potentially grouping these.

[cpennington]

@pmitros can you squash these commits? Otherwise, 👍 from me.

[pmitros]

@marcotuts I added the suggested text. I also changed 'current courses' to 'courses' since my list includes things dating back to the first semester of edX. I don't know css, so at some point (perhaps in a future PR), it'd be good to do a cleanup. I also changed to values_list as per @cpennington.

[marcotuts]

@pmitros those changes make sense. I'm also happy to add a quick commit later today with any suggested CSS changes prior to merge. Thanks!

[cpennington]

Nitpick: keyword arguments to functions shouldn't have spaces around =

[pmitros]

@marcotuts Thanks. Waiting on CSS commit... Is it still on track for this evening?

[marcotuts]

Didn't get a chance to see this today, but I added a design task for next week.

[pmitros]

@marcotuts Do you mind if I merge as-is, and you do a separate PR next week? I'd rather not leave this hanging. I'm happy to include/omit the HTML changes, as you prefer.

[marcotuts]

I haven't had a chance to review what it looks like, though I suppose other people have given it a thumbs up. If you'd like to make sure it makes it into next weeks release I can do he CSS changes now. Are tests passing now? ( can't tell I'm on mobile)

[pmitros]

@marcotuts Four tests are failing.

Two are in Studio, and appear unrelated to this patch (the same set of tests is failing in several of the other PRs).

Two are related to front-end changes and i18n (specifically, the rename of 'current courses' to 'courses'). Those are trivial to fix, but I'm not sure it makes sense to fix until CSS/HTML is in final shape since they might just need to be fixed again, depending on changes.

I know my schedule next week, and I'd basically like to get this off of my plate this weekend. If you'd allow, my preference would be to do one of four things:

1. Merge without front-end changes. Course order change, but students won't see the message about sorted by activity. We clean up with a separate PR next week to add that.
2. You do a quick sanity-check on my CSS, since I'm clueless about CSS+HTML, just to make sure I didn't do something browser-specific. We fix the tests or revert to 'Current courses,' and we merge. We clean up with a separate PR next week.
3. You fix CSS+HTML at some point tonight or over the weekend. We merge by Monday.
4. I hand this off to you. You merge at your leisure.

I suppose other people have given it a thumbs up.

Thumbs up was pre-HTML changes. None of the reviewers have looked at the front-end changes, and I am not a web designer. I would not trust my CSS/HTML without review. My preference would be towards #1 as a result (unless you have time for #3 or #4).

[marcotuts]

Hi Piotr, I'm happy to take this over and make sure it merges (#4 above) and can also look into the test failures.

[pmitros]

@marcotuts Thank you!

[singingwolfboy]

Looks like this pull request needs to be rebased.

[pmitros]

@marcotuts Ping?

[talbs]

Hi, all. This is not waiting on UX - my tasks have been completed as noted by previous comments. I'm not sure where this stands technically. Can anyone help get the right eyes on this?

[pmitros]

@talbs UX is shepherding this through the PR process. See discussion May 15/16. Until this is closed (either way), this is waiting on UX.

[talbs]

@pmitros, thanks for the reminder of the history in this epic pull request. While @marcotuts promised to help get this through, the domain expertise and decision points needed now are not owned by the UX Team.

I have compensated for the UX-related issues and concerns with your work and Product has signed off on my work and the current state of the user-facing aspect of this.

For the sake of clarity and in the spirit of moving this forward, let's skip the technical UX middleman you identified and use the Github labels as Open edX is intending to, to identify real blockers and dependencies.

From what I can tell, that has to do with database and performance issues. @sarina or @singingwolfboy, who do recommend roping in as a domain expert to help with that? Thanks for your help in getting this work to completion.

[sarina]

I'm going to say that this will need to be approved by a Product person, and tag the Platform team to help with the performance issues. Thanks for the nudge, @talbs

[ormsbee]

I think we're ok with performance and the database at the moment, we just need to resolve the test failure/dependency.

[ormsbee]

@sarina: it's already been blessed by product.

[cpennington]

Retriggering the build

[sarina]

What's going on with this PR? Tests failing, has merge conflicts.

[ormsbee]

The last comments were to make it skip the test so it doesn't explode on
the dependency. I think that's the only blocker from merging.

On Tue, Sep 16, 2014 at 1:52 PM, Sarina Canelake notifications@github.com
wrote:

What's going on with this PR? Tests failing, has merge conflicts.

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/3747#issuecomment-55784137.

[singingwolfboy]

I fixed up the tests for this pull request. Are we good to merge?

[ormsbee]

👍

[andy-armstrong]

@singingwolfboy @ormsbee @sarina @jzoldak @cpennington It seems that this change breaks on stage:

Traceback (most recent call last):  
File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/newrelic-2.18.1.15/newrelic/hooks/framework_django.py", line 492, in wrapper return wrapped(*args, **kwargs)
File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/contrib/auth/decorators.py", line 20, in _wrapped_view return view_func(request, *args, **kwargs)
File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/utils/decorators.py", line 91, in _wrapped_view response = view_func(request, *args, **kwargs)
File "/edx/app/edxapp/edx-platform/common/djangoapps/student/views.py", line 483, in dashboard course_enrollment_pairs = list(get_course_enrollment_pairs(user, course_org_filter, org_filter_out_set))
File "/edx/app/edxapp/edx-platform/common/djangoapps/student/views.py", line 274, in get_course_enrollment_pairs pairs.sort(key=key_function, reverse=True)
TypeError: can't compare offset-naive and offset-aware datetimes

See this bug report for more details:
https://openedx.atlassian.net/browse/LMS-11464