Skip to content

Add link in lms legacy instructor dashboard to cms course_index - #859

Merged
jbau merged 3 commits into
masterfrom
jbau/studio-link-in-instructor-panel
Sep 4, 2013
Merged

Add link in lms legacy instructor dashboard to cms course_index#859
jbau merged 3 commits into
masterfrom
jbau/studio-link-in-instructor-panel

Conversation

@jbau

@jbau jbau commented Sep 4, 2013

Copy link
Copy Markdown

A one-click way of getting to editing the course from the LMS instructor panel.

@sarina do you mind doing giving me comments on PR (or point me towards others)? The test coverage, etc is all there.

Basically, we've had faculty request an easier way of getting to edit course content from the instructor panel in LMS. I think there's more specific links for editing (in github) if the content is xml-backed, but linking to the CMS course_index would be enough to satisfy our faculty.

I realize it uses the legacy dashboard, but maybe that makes the merge considerations less stringent? =)

A one-click way of getting to editing the course
@sarina

sarina commented Sep 4, 2013

Copy link
Copy Markdown
Contributor

I'm concerned that this is not the right approach for us because of the way the Drupal site handles URLs (it's not as simple as what is done on Edge). I've asked someone on the Studio team to take a look.

This introduces some pep8 and pylint violations - please address those.

Comment thread lms/djangoapps/courseware/courses.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the host or can it be a relative url? Can you get the url via Location().url() v this construction?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does need the host because it's used for an external link (for the LMS to link to CMS). It's similar in spirit, (but in opposite direction) as https://github.com/edx/edx-platform/blob/master/cms/djangoapps/contentstore/utils.py#L92

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and sure, I can switch to a better utility function.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmitchell actually, having looked at Location().url(), I don't think that's actually the right thing, mainly because the addressing scheme of the URL it returns is internal rather than a standard external url. eg:

In [8]: Location(loc_or_tag="internal", org="org", course="course", category="cat", name="name").url()
Out[8]: 'internal://org/course/cat/name'

I'd have to parse out the "protocol" part of that URL anyway to make it into an actual working URL for 'course_index' in CMS (which doesn't take location as is), so I'm thinking that not much is gained by existing library functions here. But maybe I'm missing some extra support for "course" modules like they exist for "static" ones?

Basically, what I really wanted to do was a django.core.urlresolvers.reverse, but couldn't because it was across different applications and settings files

@sarina

sarina commented Sep 4, 2013

Copy link
Copy Markdown
Contributor

I'm not sure I'm loving the placement of the link.

screen shot 2013-09-04 at 11 10 34 am

Would it be possible to put it underneath the "Try New Beta Dashboard" link?

@sarina

sarina commented Sep 4, 2013

Copy link
Copy Markdown
Contributor

Somehow I manage to fix, then break, the email thing for XML backed courses. Since you define the new is_studio_course variable in this PR, I added a new commit that fixes things rather than make new logic that you'd then have to rebase and fix up around.

@jbau

jbau commented Sep 4, 2013

Copy link
Copy Markdown
Author

Thanks! Will address the other comments in short order. Sorry 'bout the PEP8 / Pylint sloppiness.

Jason

On Sep 4, 2013, at 8:33 AM, Sarina Canelake notifications@github.com wrote:

Somehow I manage to fix, then break, the email thing for XML backed courses. Since you define the new is_studio_course variable in this PR, I added a new commit that fixes things rather than make new logic that you'd then have to rebase and fix up around.


Reply to this email directly or view it on GitHub.

* Uses override_settings to provide test variable
* Move location of cms link to upper right
* PEP8 / Pylink
@jbau

jbau commented Sep 4, 2013

Copy link
Copy Markdown
Author

ok I repushed changes for the test and where the link appears @sarina @singingwolfboy

@sarina

sarina commented Sep 4, 2013

Copy link
Copy Markdown
Contributor

screen shot 2013-09-04 at 3 45 20 pm

👍

@jbau

jbau commented Sep 4, 2013

Copy link
Copy Markdown
Author

@dmitchell gave his blessing via hipchat.

jbau added a commit that referenced this pull request Sep 4, 2013
Add link in lms legacy instructor dashboard to cms course_index
@jbau
jbau merged commit 08168ec into master Sep 4, 2013
bradenmacdonald referenced this pull request in open-craft/openedx-platform Sep 1, 2017
ziafazal pushed a commit that referenced this pull request Dec 12, 2017
andrey-canon pushed a commit to eduNEXT/edx-platform that referenced this pull request Aug 16, 2018
caesar2164 added a commit to caesar2164/edx-platform that referenced this pull request Dec 19, 2018
…oning

Update version of Platform Tour to 1.0.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants