feat: adds a certificate template modifier #34042
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adds a management command to modify certificate templates, with a dry run option. Inherently unsafe; just as a search and replace on the first string match. TODO: * unit tests * make sure the multi-line string replacements work via django admin FIXES: APER-2851
…cate-in-facebook-improvements-for-edx.org
undid my auto formatters reordering of the includes, because I'm assuming that's what the lint-amnesty directives are there for. Best practice would have a human-written comment explaining why they need to stay there, but I'm just going have to assume it's correct, Because no such human-written comment exists. FIXES: APER-2851
fixing a long line problem FIXES: APER-2851
adding the migration for the ability to manage these command configs via django admin FIXES: APER-2851
pep8 FIXES: APER-2851
…cate-in-facebook-improvements-for-edx.org
still needs mocks for all tests to work FIXES: APER-2851
…cate-in-facebook-improvements-for-edx.org
…cate-in-facebook-improvements-for-edx.org
Re-factored to make the code more testable, and added some more tests, also improved dry run logging FIXES: APER-2851
…cate-in-facebook-improvements-for-edx.org
deborahgu
commented
Jan 17, 2024
| class Meta: | ||
| model = GeneratedCertificate | ||
|
|
||
| course_id = None | ||
| status = CertificateStatuses.unavailable | ||
| mode = GeneratedCertificate.MODES.honor | ||
| name = '' | ||
| name = "" |
There was a problem hiding this comment.
Quote style changed to our approved doublequotes on saving the file.
deborahgu
commented
Jan 17, 2024
|
|
||
| generate_course_certificate(user=student, course_key=course_key, status=status, enrollment_mode=enrollment_mode, | ||
| course_grade=course_grade, generation_mode=generation_mode) | ||
| student = User.objects.get(id=kwargs.pop("student")) |
There was a problem hiding this comment.
changed to standard style on file save.
not sure why the manual linter didn't complain FIXES: APER-2851
Fixing a linter air FIXES: APER-2851
justinhynes
reviewed
Jan 18, 2024
Comment on lines
+23
to
+31
| This is DANGEROUS. | ||
| * This uses string replacement to modify HTML-like templates, because the presence of | ||
| Django Templating makes it impossible to do true parsing. | ||
| * This isn't parameterizing the replacement text, for the same reason. It has | ||
| no way of knowing what is template language and what is HTML. | ||
| Do not trust that this will get the conversion right without verification, | ||
| and absolutely do not accepted untrusted user input for the replacement text. This is | ||
| to be run by trusted users only. | ||
| Always run this with dry-run or in a reliable test environment. |
There was a problem hiding this comment.
Kudos 👍. Thanks for pointing out that this is a potentially dangerous and destructive action.
justinhynes
reviewed
Jan 18, 2024
| class Command(BaseCommand): | ||
| """Management command to modify certificate templates. | ||
| Example usage: | ||
| ./manage.py lms modify_cert_template --old-text |
There was a problem hiding this comment.
The example seems incomplete at a first glance. Should this also have an example of --old-text and how to use the other options (like --new_text {blah})? I'd love to see an example of how to specify the templates too.
justinhynes
approved these changes
Jan 18, 2024
improving a comment FIXES: APER-2851
deborahgu
deleted the
dkaplan1/APER-2851_replicate-share-certificate-in-facebook-improvements-for-edx.org
branch
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds a management command to modify certificate templates, with a dry run option.
Notes:
Inherently unsafe; just as a search and replace on the first string match, because you can't use HTML/
lxml-style parsing on HTML blended with django templating instructions. For the same reason, doesn't do any sanity checking on the contents of the string match, except for the safety checking that's inherent on the template itself. Help/usage text stresses that it should be always run with--dry-runfirst.FIXES: APER-2851