[BB-5819] Make course description editable in certs

[pkulkark]

Description

This PR makes the default description text that appears under the course title editable/configurable for certificates.

Supporting information

JIRA tickets: BB-5819

Screenshots

Creating new certificates:

Testing instructions

1. Check out this branch and open Studio.
2. Enable certificates on the a course.
3. On clicking "Set up your certificate", you should see the additional field "Course Description".
4. Add in some text for course description and preview certificate.
5. Verify that the custom course description is shown on the certificate.

Reviewers:

• @gabor-boros
• edX reviewer: @hurtstotouchfire

[openedx-webhooks]

Thanks for the pull request, @pkulkark!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

[gabor-boros]

@pkulkark actually, setting the default value to empty means that we take away the default text. I think this is something we wouldn't like to do as it could cause confusion -- why isn't the a course of study offered by {partner_short_name}... text is there.

I would suggest having the course description as an override. If the value is set, we don't care about the default text, otherwise we default to the removed condition.

[pkulkark]

Good point. Made the changes.

[gabor-boros]

Thank you for the changes @pkulkark, just a small nit, feel free to ignore it. It may not help the formatting a lot at all. How about this?

course_description_override = context['certificate_data'].get('course_description')

if course_description_override:
    accomplishment_copy_course_description = course_description_override
elif context['organization_long_name']:
    # Translators:  This text represents the description of course
    accomplishment_copy_course_description = _(
        'a course of study offered by {partner_short_name}, '
        'an online learning initiative of '
        '{partner_long_name}.'
    ).format(
        partner_short_name=context['organization_short_name'],
        partner_long_name=context['organization_long_name'],
        platform_name=platform_name,
    )
else:
    # Translators:  This text represents the description of course
    accomplishment_copy_course_description = _('a course of study offered by {partner_short_name}.').format(
        partner_short_name=context['organization_short_name'],
        platform_name=platform_name,
    )


context['accomplishment_copy_course_description'] = accomplishment_copy_course_description

[pkulkark]

Added it

[gabor-boros]

👍 🎉 The changes are working great by the way!

I tested what's in the test instructions
I read through the code

[pkulkark]

@natabene This is good for edX's review.

[natabene]

@pkulkark Thank you for your contribution.

[hurtstotouchfire]

Thanks for your review @gabor-boros, it was very helpful to read through. I will try to get eyes on this and merge it soon.

[hurtstotouchfire]

I'm hesitant to add complexity to this feature that we want to extract to the credentials service in the long run. If you have specific needs for this feature would you be willing to participate in some roadmap planning we are going to be starting?

[0x29a]

@pkulkark, @gabor-boros, do we have any plans on this PR? Asking, because we carry it in our shared branch from a release to release.

[mphilbrick211]

@pkulkark @gabor-boros Just checking in on this to see if there are plans to move forward with this?

[gabor-boros]

Hello @mphilbrick211,

As @pkulkark is the author and I'm no longer on that given project that this PR belongs to, @pkulkark do we still need this PR?

[pkulkark]

@mphilbrick211 This was put on hold because of the plan to move course certs to credential service. But it looks like that's going take a while. It's not a very high risk change so we could probably move ahead with this if there's no objection. What do you think?

[mphilbrick211]

Hi @hurtstotouchfire - please see the comment above from @pkulkark - I'm not sure if this should move forward, could you please advise? It looks like you requested some changes a while back.

[mphilbrick211]

@mphilbrick211 This was put on hold because of the plan to move course certs to credential service. But it looks like that's going take a while. It's not a very high risk change so we could probably move ahead with this if there's no objection. What do you think?

@hurtstotouchfire - flagging this comment again to see if this should move forward. Thanks!

[mphilbrick211]

@hurtstotouchfire just checking to see if this is still being pursued? If so, we'll need to have Product take a look as well.

[jmakowski1123]

@mphilbrick211 Imo this could move forward. As @hurtstotouchfire notes, it's a low risk change. We have all the requisite info needed for product review here, and the next step would be for me/Ryan to assign it to Claire Zhang to complete the product review.

[hurtstotouchfire]

@mphilbrick211 Sorry, as you may have noticed I miss a lot of github pings due to signal to noise issues, and it's best to ping me in slack.

From my perspective, this adds an additional field which would need to be migrated to Credentials when we extract course certs. Description is also a larger and more complex field than title, so it's a step beyond the current precedent in terms of complexity. Here's what I would like to see from product:

1. Demonstrate that this feature is critically needed and worth adding complexity to the extraction of course certs, keeping in mind the following engineering constraints:
   • extracting course certs is already so complex that no one has been willing to commit to taking on the project (this would argue for refraining from adding any additional complexity)
   • in order to get the project down to a manageable level of complexity, we will likely need to cut some of the edge cases that are currently supported by the LMS certificates app, and this case might be cut anyway (this might argue for going ahead and supporting this in the legacy app, with the understanding that it may not be supported in the future, extracted app)
2. Spec out exactly what the description over-ride field would support and, if possible, limit the complexity. This will help make it more feasible to support going forward.
   • Does it have a character limit?
   • Does it support html?
   • Does it support JavaScript?
   • Does it support django templating commands?

[mphilbrick211]

Hi @pkulkark! We've had a couple new tests pop up that need to be run (e.g. "shellcheck") - would you mind re-running these while this PR is in product review? Thanks!

[mphilbrick211]

Hi @pkulkark! We've had a couple new tests pop up that need to be run (e.g. "shellcheck") - would you mind re-running these while this PR is in product review? Thanks!

Hi @pkulkark - just flagging this!

[pkulkark]

@mphilbrick211 Sorry for the delay in getting back on this. I've just rebased it to latest master and started the tests.

[pkulkark]

@mphilbrick211 Just following up on this - can this be merged soon?

[mphilbrick211]

hi @pkulkark - I believe this is still under Product review. Flagging for @openedx/2u-aperture and @jmakowski1123

[hurtstotouchfire]

This is still very far from viable from my perspective. I would still like to see a clear set of requirements from Product and a sense of the level of need, but from there we would also need to work through several other questions about the particulars of how this feature should work, such as:

• What does the field need to support? HTML? Django templating commands? Does it have a character limit?
• How would this behave in existing templates? The OSPR updates the base templates but any existing templates in live instances will need to be manually updated to support this field.
• Given that it requires updating all of your current templates to support this field, operators should have the ability to disable this feature if they do not plan to support it.

I think there is probably more that would come up if we thought through the requirements for this and really laid out all of the implications. But generally speaking, I don't feel this is production-ready as a feature.

[jmakowski1123]

openedx/platform-roadmap#227 (comment)

[openedx-webhooks]

Thanks for the pull request, @pkulkark! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

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

• supporting documentation
• Open edX discussion forum 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 be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[mphilbrick211]

openedx/platform-roadmap#227 (comment)

Closing for now per Jenna's comment.

[openedx-webhooks]

@pkulkark Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.