Skip to content

Show a success button at the top of the Progress page - #2780

Merged
singingwolfboy merged 2 commits into
openedx:masterfrom
FiloSottile:progress_success_button
Apr 25, 2014
Merged

Show a success button at the top of the Progress page#2780
singingwolfboy merged 2 commits into
openedx:masterfrom
FiloSottile:progress_success_button

Conversation

@FiloSottile

Copy link
Copy Markdown
Contributor

Add SHOW_PROGRESS_SUCCESS_BUTTON feature

This will show a button at the top of lms/templates/courseware/progress.html if the lowest nonzero grade cutoff has been reached.

Introduce two settings:

  • PROGRESS_SUCCESS_BUTTON_URL is the href of that button (the course id is appended)
  • PROGRESS_SUCCESS_BUTTON_TEXT is the text, defaults to "Download your certificate"

The change is pretty minimal. However, feedback on settings naming/placement and testing would be welcome.

This is a test task for recruiting purposes - @antoviaque

@antoviaque

Copy link
Copy Markdown
Contributor

@singingwolfboy Who would be a good person to review this? You? : ) While this is an optional feature, it might be worth checking if/how it would make sense from a product perspective - it's used by a client to get his students to his own website, where he generates the certificates.

@FiloSottile This PR is getting a bit old, it might be useful to rebase to get a proper review.

Comment thread lms/envs/common.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.

It would be better to just put this text directly in the template, so that it can be internationalized effectively.

@singingwolfboy

Copy link
Copy Markdown
Contributor

You might want to take a look at our public certificate generation code: maybe you can make this pull request work with it somehow?

@FiloSottile

Copy link
Copy Markdown
Contributor Author

Rebased and addressed @singingwolfboy's remarks. See commit message 3eb139951af03f7547710503c6e8fd5b93520697

Doubt: should I urlencode the course_id and student_id fields? (@antoviaque)

Comment thread lms/templates/courseware/progress.html 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.

I much prefer str.format() to the old-style % formatting. Can you switch it to the new version?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

Show a button at the top of lms/templates/courseware/progress.html if the
lowest nonzero grade cutoff has been reached. Introduce two settings:
PROGRESS_SUCCESS_BUTTON_URL is the href of that button (the course id is
appended); PROGRESS_SUCCESS_BUTTON_TEXT is the text, defaults to
"Download your certificate"
The button text default is now in the template and i18n-able, a
PROGRESS_SUCCESS_BUTTON_TEXT_OVERRIDE option is offered in settings.
The button url now uses string formatting.
@FiloSottile

Copy link
Copy Markdown
Contributor Author

@singingwolfboy Implemented your comments, except for the one about the OVERRIDE option for the reason @antoviaque explained here https://github.com/edx/edx-platform/pull/2780#discussion_r11098095, and rebased.

@FiloSottile

Copy link
Copy Markdown
Contributor Author

@singingwolfboy Sorry to ping you again, any new comments?

@singingwolfboy

Copy link
Copy Markdown
Contributor

Sorry for the delay. This seems fine to me: merging.

singingwolfboy added a commit that referenced this pull request Apr 25, 2014
Show a success button at the top of the Progress page
@singingwolfboy
singingwolfboy merged commit 5edae3e into openedx:master Apr 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants