ECOM-7388 Program details page redesign

[MatthewPiatetsky]

Pretty much all the files are new. The idea is there are old files and new files with same name and a _2017 suffix. When we're ready to remove the old code, we just delete all the old files, and remove the _2017 from the new files.
Most of the changes are in underscore templates and css.
Most of the views with _2017 are nearly the same as the old ones, just renamed so we can clean up easily.

Demo
email: mpiatetsky@edx.org
password: edx

Tasks

• a11y requirements review
• finalize responsive design
• final design review
• analytics
• browser testing
• • Chrome
• • Safari
• • Mobile Safari
• • Firefox
• • Edge
• • IE 11
• test button functionality
• tests
• verify edge cases where data is missing
  (missing dates, empty course sections)

[AlasdairSwan]

I would remove the id and data-name attributes, and the <defs> (including the styles). Could you add a more meaningful class than cls-1 and add a default fill colour to Sass.

[AlasdairSwan]

I would remove the id and data-name attributes, and the <defs> (including the styles). Could you add a more meaningful class than cls-1 and add a default fill colour to Sass.

[AlasdairSwan]

I would remove the id and data-name attributes, and the <defs> (including the styles). Could you add a more meaningful class than cls-1 and add a default fill colour to Sass.

[AlasdairSwan]

Dot notation is preferred (e.g. seat.type).

[AlasdairSwan]

Dot notation please.

[AlasdairSwan]

Please use a Sass variable.

[AlasdairSwan]

Please use a Sass variable.

[AlasdairSwan]

Please use a Sass variable.

[AlasdairSwan]

Please use a Sass variable.

[AlasdairSwan]

Please use a Sass variable.

[rlucioni]

There's going to be a lot duplicated between program_data and courses_progress. The bulk of program_data is data about the courses in the program, and it looks like that same data is going to end up grouped in course_progress. What can we do about that?

I'd update the tests of this view so that they verify the data is present on the page. Also, if these end up staying separate, a more descriptive name might be nice here. course_groups, for example.

[MatthewPiatetsky]

well i think the program data is primarily used for the program data and this is just the course data.
Assuming we didn't need the course data we could theoretically remove it from the program_data object but that doesn't seem necessary to me

[rlucioni]

@MatthewPiatetsky the course data is the majority of the program data. If nothing changes here, we'll send that data to clients twice for no reason. We shouldn't increase the size of the response more than we have to.

[MatthewPiatetsky]

are you suggesting we just delete the course data from the program data object if it is in fact unused?

[rlucioni]

@MatthewPiatetsky not necessarily, I'm just asking you to find a way to avoid increasing the size of the response unnecessarily. That could mean popping the course data from the program data dict if it's not going to be messy.

[rlucioni]

Is this necessary?

[rlucioni]

A test that covers the case where count_only is False would be good.

[MatthewPiatetsky]

added

[AlasdairSwan]

Nitpick: I'm not sure it matters that they are logos or what the name of the color is (e.g. if next month they wanted to make the xseries color orange) so I would consider renaming them $micromasters-color, etc.

[AlasdairSwan]

I would move this down a few lines so it's after the initial margin declarations. Also, an alternative way of writing this is;

margin: {
    left: 30px;
    right: 80px;
}

[AlasdairSwan]

Variables should be used for colours.

[AlasdairSwan]

Variables should be used for colours.

[AlasdairSwan]

Variables should be used for colours.

[AlasdairSwan]

Variables should be used for colours.

[AlasdairSwan]

This should either be margin-bottom: 5px or margin: 0 0 5px 0;

[AlasdairSwan]

Variables should be used for colours.

[AlasdairSwan]

Variables should be used for colours.

[AlasdairSwan]

Variables should be used for colours.

[AlasdairSwan]

Variables should be used for colours.

[AlasdairSwan]

Variables should be used for colours.

[mikedikan]

looks pretty good so far - minor comments/questions

[mikedikan]

can you leave a comment explaining why we ignore the linter here?

[MatthewPiatetsky]

I am using filter and not returning all seat types (which means the function doesn't always return.)
Not sure, maybe i can return nothing to make the filter ignore it or something like that

[mikedikan]

that sounds reasonable

[mikedikan]

USD is the only currency where the sign precedes the value?

[MatthewPiatetsky]

for the other currencies we just have the name.

It would be weird to put something like USD10. This is basically only handling USD and adding a relatively sane fallback behavior for other currencies until we know how we want to deal with them

[mikedikan]

👍

[mikedikan]

what is the 's' after the start_date variable?

[MatthewPiatetsky]

@rlucioni do you know?

[AlasdairSwan]

@mikedikan @MatthewPiatetsky it's the wrapper for vars that interpolate expects Starts: %(my_var)s

[mikedikan]

@MatthewPiatetsky can you update the demo link so it uses your IP?

[MatthewPiatetsky]

@AlasdairSwan i think i've either commented on or addressed most of your comments.
Also, do you know where I would look to add tests for this page?

[AlasdairSwan]

@MatthewPiatetsky here are the test for the current program dashboard https://github.com/edx/edx-platform/tree/26201161813a15b565ffdccd91c1ea5ee9a01b0e/lms/static/js/spec/learner_dashboard

[AlasdairSwan]

All 3 SVGs still have class cls-1, could we change it to be more meaningful?

[AlasdairSwan]

What is this line doing? !([undefined, 'None', null].indexOf isn't very clear.

[MatthewPiatetsky]

this line is checking that courseRun.advertised_start is not undefined, 'None' or null
do you have suggestion for making this more clear?

[AlasdairSwan]

if (!_.isUndefined(course_run.advertised_start))

[MatthewPiatetsky]

that works for the undefined case but not the other two I think?

[AlasdairSwan]

Yeah, you're right. The 'None' value is a weird one. I think it would be clearer what's going on in the code if you created a function called something like isDefined that does the check i.e.

isDefined: function(str) {
    return [undefined, 'None', null].indexOf(str) >= 0;
}

[AlasdairSwan]

@MatthewPiatetsky I would still prefer this check to be its own function to make the code easier to read.

[MatthewPiatetsky]

thanks changed

[AlasdairSwan]

Would it make more sense to default the value of enrolled to false?

[MatthewPiatetsky]

at the moment i was passing the text to display to the template. would it be better if it was a boolean that that the template looked at with an if block?

[AlasdairSwan]

That's fine, I was assuming it was a boolean and not text.

[AlasdairSwan]

Yeah, I think it makes sense to move it.

[AlasdairSwan]

Shouldn't this be wrapped in an if (this.remainingCourseCollection.length > 0) { ?

[AlasdairSwan]

@mikedikan @MatthewPiatetsky it's the wrapper for vars that interpolate expects Starts: %(my_var)s

[AlasdairSwan]

Why 2 versions of this file?

[AlasdairSwan]

Shouldn't this just be <% if (!is_enrolled) { %>?

[MatthewPiatetsky]

@AlasdairSwan if i comment out these images my program_details_view_spec.js tests work
otherwise the tests error on a 404 on these. what is the correct way to include them in the spec file?

[AlasdairSwan]

That's weird, I didn't think we have to do anything special because the same text! plugin is being used for underscore templates. Maybe the karma.conf isn't aware of the folder the images are in?

[MatthewPiatetsky]

we already had this file, so this is almost a carbon copy with some small tweaks for the new code (the other spec file is brand new)

[MatthewPiatetsky]

@AlasdairSwan @rlucioni bump

[rlucioni]

We don't want this method's output to change across the list and detail views. The user's view of progress should be consistent across those pages. This should just be how the method behaves, not an option.

We're also at risk of introducing debt here. It's not safe to assume that just because the upgrade deadline is past, completion of the course run will no longer count for completion of the course. Each program has a type, and those types define a set of seat types that qualify for completion of the program. Given that we have support for that, we shouldn't be hardcoding assumptions that all programs require verified seats like you're doing here.

At the very least, this should behave like the ProgramProgressMeter's completed_programs property. Read this.

[rlucioni]

Please use intelligible variable names.

[rlucioni]

Is passing tzlocal() required? Django should take care of timezone detection for you.

[rlucioni]

This is only going to work for an "extended_program." In other words, this will only work if you provide extended programs when initializing the class. Expecting people to know that when using this method worries me.

Furthermore, how are these course runs ordered? My sense is that it's not safe to just pick the first one. Consider the case where the user is enrolled in two runs of the same course. This may pick the older enrollment and miss the newer one.

[rlucioni]

This variable name can be better; this isn't a list of seats.

[rlucioni]

We use a specific format for events that we should stick to unless there's a good reason not to. Refer to the events in the edx.org GA account for an example.

Also: I don't think the courseRun property you're including here is going to be surfaced in GA. It may not be worth including.

[MatthewPiatetsky]

ok, there are a bunch in different formats like 'Completed Purchase', but i'm assuming you mean the edx.bi format
i'll go with edx.bi.user.program-details.enrollment

[MatthewPiatetsky]

@rlucioni you asked to bump you again

[AlasdairSwan]

The svgs are still using cls-1 as their class, please update to something more descriptive.

[MatthewPiatetsky]

thanks fixed, before when i addressed your comment i didn't realize there were multiple instances of these classes in this file

[AlasdairSwan]

@MatthewPiatetsky I would still prefer this check to be its own function to make the code easier to read.

[AlasdairSwan]

Nitpick: the code would be easier to read if spread across a few lines, e.g.

var data = $.extend(this.model.toJSON(), {
    enrolled: this.context.enrolled || ''
});

[MatthewPiatetsky]

changed

[AlasdairSwan]

Are we still loading multiple versions of an image for responsive? If not we can remove this (and any other picturefill references).

[MatthewPiatetsky]

good call, i don't think this is necessary anymore

[AlasdairSwan]

Why these changes?

[MatthewPiatetsky]

accidental commit, removed

[AlasdairSwan]

👍

[rlucioni]

@AlasdairSwan you may want to re-use this for the sidebar ring.

[rlucioni]

Is it easy to do this without hardcoding the names of the program types? We've been over why we try to avoid this before, so I won't elaborate.

[MatthewPiatetsky]

not sure about this, asked Alasdair

[AlasdairSwan]

It's going to be difficult because of the way we are passing in the SVG logos. I suppose we could have a logoCollection, and we could do logoCollection.findWhere({type: type}) so at least we are moving the hard-coded program types into the logo Collection.

[MatthewPiatetsky]

@rlucioni do you think that would be better? we would still have to hardcode the program types

[rlucioni]

@MatthewPiatetsky no, it doesn't really solve the problem, just moves it elsewhere. Maybe it's not worth worrying about here, but I still think we should be aware of it.

[AlasdairSwan]

The only other solution I can think of would be for the API to return the SVG.

[rlucioni]

@AlasdairSwan good idea. The discovery API is currently capable of returning a link to an image for each program type inline with the serialized programs, and I could see us returning a link to an SVG served by discovery/S3. Right now there are PNGs logos for the program types stored in the discovery service.

Let's table this for now, but I (and @clintonb) like the idea of using the logos from the discovery service where possible instead of duplicating them in other services.

[MatthewPiatetsky]

the complicating issue there is we may have a legitimate need to have different logos in different places (which we currently do). so it wouldn't be as simple as just storing a single logo. The logos would have to be tied somehow to where they would be used

[rlucioni]

@MatthewPiatetsky yeah, that's a good point. Though I don't think it prevents discovery from providing an API that can get you the right image for the right context.

[rlucioni]

What if an open source user is trying to run this? The default is just not to show a logo at all?

[MatthewPiatetsky]

that was my assumption. we don't have a default logo.

[rlucioni]

Okay, just making sure I understand the intention. Seems fine to me.

[rlucioni]

Nitpick: missing newline.

[MatthewPiatetsky]

fixed

[rlucioni]

Nitpick: please leave this trailing comma, it's good practice. It leaves the next person to touch this dictionary with a cleaner diff.

[MatthewPiatetsky]

fixed

[rlucioni]

Please remember to squash your commits!

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, April 11, 2017.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.