-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add functionality to create honor course modes through studio #28114
Conversation
Thanks for the pull request, @mariajgrimaldi! 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:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate. |
80ccba0
to
650f176
Compare
@mariajgrimaldi Thank you for your contribution. Please let me know once it is ready for our review. |
650f176
to
df75ee1
Compare
Hey @natabene! This is ready for review :) |
0446fc2
to
c3e7787
Compare
Do you mind including a screenshot for what this looks like? My initial reaction is that I dont think we want to configure enrollment tracks in the same UI as certificate configuration but instead a separate area to review / edit enrollment tracks but I'm happy to look at what the existing experience is first if possible via screenshots. Thanks @mariajgrimaldi |
c3e7787
to
c0f8384
Compare
Thank you for the reminder, @marcotuts! I'll describe in detail the testing process while illustrating the functionality:
Go to your course advanced settings and then show deprecated settings: |
c0f8384
to
bbd0007
Compare
bbd0007
to
7fd20ba
Compare
Hi @marcotuts, what if we add an |
Your PR has finished running tests. The following contexts failed:
|
8dc89f5
to
aba0bf6
Compare
@marcotuts When you have a chance, can you reply? |
Hi @mariajgrimaldi ! Thanks so much for your patience, and sorry for the delay. We're working out the best workflow for getting PRs through product review. Yes, I think your proposal to add an "Edit Enrollment Tracks" into the Main Settings tab in Studio is a viable option. I have two suggestions to move this PR forward, if you are still interested in pursuing :) First, I think we should expand the scope to allow course teams to add/change a more comprehensive list of course modes, and not limit it to just the Honor course mode. If we are going to add a new setting in Studio for this, I think it makes sense to review the entire course mode selection listing, rather than just implementing it for one mode type. Second, this will need a UI design in alignment with the most current Paragon Design System. Since this new proposed scope of work is slightly larger than this PR, I've created a Product feature ticket for this on the Open edX Community Roadmap. We can use that Product ticket to communicate and nail down the full Product Spec in order to get it approved. I'm happy to work with you to land the decision about which course modes to include, and to facilitate review of UI. Let's shift the conversation over to that ticket for now. Cc @mphilbrick211 |
@jmakowski1123: I agree with both of your suggestions.
I'm guessing the next step is nailing down the course modes to include, so I'll move to the product ticket so we can discuss it :) |
Great! I've got a start on the product feature ticket here: openedx/platform-roadmap#228 |
aba0bf6
to
98b1d0c
Compare
}); | ||
var username = $('.account-username')[0].innerText; | ||
$.ajax({ | ||
url: this.courseModeCreationUrl + '?username=' + username, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to author: how can we update the course mode API so we don't necessarily have to send the username
mode_slug: 'honor', | ||
mode_display_name: 'Honor', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to author: here we need a dictionary or list[tuple] of course modes
course_id: this.courseId, | ||
mode_slug: 'honor', | ||
mode_display_name: 'Honor', | ||
currency: 'usd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to author: is there a setting for this? Search.
@@ -0,0 +1,6 @@ | |||
<div class="no-content"> | |||
<label for="add-course-mode"><%- gettext("Enable the honor mode for the course.") %></label> | |||
<a href="#" class="button new-button add-course-mode"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to author: does this class use the new design system?
path( | ||
'api/course_modes/', | ||
include( | ||
('common.djangoapps.course_modes.rest_api.urls', 'common.djangoapps.course_mods'), | ||
namespace='course_modes_api', | ||
) | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self: why is this the best way of doing it?
# Check whether the audit mode associated with the course exists in database | ||
has_audit_mode = CourseMode.objects.filter(course_id=course.id, mode_slug='audit') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to author: modify this so it doesn't matter if audit mode exists
98b1d0c
to
a491581
Compare
I've been considering Jenna's suggestion about using the new Paragon Design System. Does that mean this new page must be included in the course authoring MFE, right? Like the new Pages & Resources. I'm thinking this since the current solution uses a backbone application, I'm not sure how that's compatible with the new design system. Someone on my team suggested checking out student_account which uses some Paragon components. But still, I'm not sure how to proceed. 🤔 I'm not sure who can answer this question, though. Would you @arbrandes or @jmakowski1123 point me in the right direction? Thanks! |
cc @dcoa |
@mariajgrimaldi, I haven't reviewed the code here, or tested it, but to answer your question:
If the feature implemented in this PR can be implemented instead in an MFE and you have the knowledge and resources, please do so! This will save us the work of having to do it later: because we'll definitely have to do it later. :) Technically, you could do it with Paragon in edx-platform, but I don't recommend it. It would be much harder to maintain. It's why we have MFEs, after all. As for where: the course-authoring MFE seems like a reasonable choice, since this is supposed to be author-facing. Which is to say, I would support an implementation in there. Since it's a new page, it would likely not affect anything else, which makes it more likely to be merged faster. Finally, it's totally fine to add endpoints to Studio that are meant to be used by authors! |
Hi @mariajgrimaldi and @arbrandes! Just checking to see if there's an update here? |
Apologies - forgot this was in product review! Left a comment on the Roadmap ticket :) |
hi @mariajgrimaldi! Per Jenna's comment here, would you mind switching this to a draft? Thanks! |
Thanks for the pull request, @mariajgrimaldi! 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:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate. |
FYI: this is not our priority now (or in the foreseeable future), if that changes, I'll leave a comment here. Either way, I'll leave the PR in draft if someone wants to pick up the work. Thanks! |
I'll close this PR for now since all these changes are related to the Studio Legacy FE, and we'd prefer the new Studio MFE to support this instead. |
@mariajgrimaldi 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. |
Description
This PR implements a different mechanism that allows the
course team
users to create honor course modes through the studio certificate view, which empowers them, among other steps, to generate certificates.Some implementation details:
To call the API, we had to add the course modes URLs to Studio. We did this after failing in communicating Studio with the LMS due to CORS errors:
We tried to solve them by modifying some Nginx and site configurations, but we decided to drop them after some failed attempts.
Do you have any suggestions?
Supporting information
The overall idea was mentioned in this jira ticket and discussed in this Discuss Post.
Testing instructions
ENABLE_COURSE_MODE_CREATION
feature flag./certificates/course_key.
Video explanation:
Deadline
None
Other information