Initial chromeless template (for LTI Provider)

[simultech]

Initially for LTI embedding, but aims to be generic and reusable. Removes some of the conditional requirements which are present in courseware.html.

[openedx-webhooks]

Thanks for the pull request, @simultech! I've created OSPR-616 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• edx-code email threads
• timeline information ('this must be merged by XX date', and why that is)
• partner information ('this is a course on edx.org')
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

We can't start reviewing your pull request until you've added yourself to the AUTHORS file. Please see the CONTRIBUTING file for more information.

[simultech]

Flagging @mcgachey

As discussed in here:
https://github.com/edx/edx-platform/pull/7624

I'm expecting this will evolve over time (especially around the common chat code), but a start that we can refine over time, gives us flexibility with the LTI view rather than being constrained & cautious by courseware.html

[sarina]

@simultech please do add yourself to AUTHORS. You may want to just open a separate PR to do that, because the AUTHORS file changes rapidly and means you have to rebase a lot.

[sarina]

I really dislike the idea of adding in a new base template if we can avoid it, because it means that we now have to keep two templates in sync with changes, and everyone has to understand and know how to test this. Is there a way to not do this?

[mcgachey]

Hi, Andrew

Could you give a little more detail about your plans around this template? Based on what we've talked about, I suspect that the duplication concern that @sarina raised shouldn't be an issue, since this new template will become the only location for this code, and the existing courseware template functionality will be recreated by embedding this chromeless template into a new template that adds the chrome back again. As it looks right now, though, it's not clear that's what will happen.

Also pinging @cpennington and @ormsbee, since they were in on the original discussion.

[sarina]

@mcgachey Well my concern is around the actual strings that are included - what if our UX changes and we instead refer to this as "The Course Ware For Course Number {course_number}" in the main template? Do we need to update this template as well? This template looks super similar to courseware/courseware.html, so when there are changes to courseware.html who's responsible for updating courseware-chromeless.html? Sorry if I didn't make my concern clear at first.

[mcgachey]

Ah - that might be a different concern than I was thinking of. You're right that any string should only exist in one place (which presumably has implications for translation as well). If they're duplicated in main.html (or somewhere else) then we should rationalize them.

The duplication between this template and courseware/courseware.html is the part that I believe will be straightened out, although it isn't in this change. The duplicated code should be removed from courseware/courseware.html, and this file will be imported there instead. That way there won't be two places to update any changes.

Apologies if I'm putting words into your mouth, Andrew - the time difference could make the conversation somewhat staggered. It might be best to write up a description of how the templates will be structured in order to address the question of duplication.

[simultech]

Apologies @sarina that I wasn't as clear as I could have been. As @mcgachey stated this is a first step towards the refactoring of the courseware.html template, and while this pull request has duplicate code, moving forward the plan is for courseware.html to instead import the content rendering from courseware-chromeless.html. I am working towards this; however, I haven't included in this request because of a few reasons:

• We are still working through the LTI (both the architecture and the UX), so there are still some unanswered questions regarding what is required of the chromeless view template and what isn't (e.g. chat, notes etc., whether LTI needs its own chrome etc.)
• At the same time we need the flexibility of being able to iterate on the LTI front-end without adding additional complexity/conditionals directly into courseware.html
• I think its safer to do a clean transition of courseware.html as a wrapper once the LTI end to end experience is worked through, rather than doing it now (and potentially end up in the same boat as we are currently in)

TLDR;
Short term - yes duplication, giving flexibility to work through the evolving LTI experience without introducing increased complexity into courseware.html
Medium term - couseware.html becomes a wrapper for courseware-chromeless.html

Thoughts appreciated.

[sarina]

I think that makes sense, and I appreciate the clarification.

I've put this into @ormsbee https://github.com/ormsbee's queue to review,
so he will weigh in when he can.

On Thu, May 28, 2015 at 7:23 PM, simultech notifications@github.com wrote:

Apologies @sarina https://github.com/sarina that I wasn't as clear as I
could have been. As @mcgachey https://github.com/mcgachey stated this
is a first step towards the refactoring of the courseware.html template,
and while this pull request has duplicate code, moving forward the plan is
for courseware.html to instead import the content rendering from
courseware-chromeless.html. I am working towards this; however, I haven't
included in this request because of a few reasons:

• We are still working through the LTI (both the architecture and the
  UX), so there are still some unanswered questions regarding what is
  required of the chromeless view template and what isn't (e.g. chat, notes
  etc., whether LTI needs its own chrome etc.)
• At the same time we need the flexibility of being able to iterate on
  the LTI front-end without adding additional complexity/conditionals
  directly into courseware.html
• I think its safer to do a clean transition of courseware.html as a
  wrapper once the LTI end to end experience is worked through, rather than
  doing it now (and potentially end up in the same boat as we are currently
  in)

TLDR;
Short term - yes duplication, giving flexibility to work through the
evolving LTI experience without introducing increased complexity into
courseware.html
Medium term - couseware.html becomes a wrapper for
courseware-chromeless.html

Thoughts appreciated.

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

[cpennington]

I'm going to be pinch-hitting for @ormsbee on this PR.

[cpennington]

Perhaps not for this PR, but it would be great if the header were in a block as well, so that the chromeless template could just override that block to be empty, rather than requiring the caller to pass in this variable.

[cpennington]

👍, but it would be great to have a clearer timeframe on when we're going to switch courseware.html over to nest the chromeless version to pay down this technical debt.

[simultech]

@cpennington I will aim for a pull request which switches over courseware.html by 30th of June (gives us time to flesh out the differences and I am at wwdc conference for a week + traveling). Does that sound reasonable?

[simultech]

Is this all good? Its still tagged as waiting on author?

[sarina]

@simultech You still have a failing build - please address test failures and once your build is passing please ping @cpennington

See https://github.com/edx/edx-platform/blob/master/docs/en_us/internal/testing.rst for running tests locally; CI builds are very expensive (and take ~1 hour), so please run tests locally and get them to pass locally before pushing new code.

[simultech]

ping @cpennington - updated the test case now.