Ziafazal/add pre requisite course via milestones app - #6126
Conversation
|
@marcotuts @smagoun, FYI |
There was a problem hiding this comment.
Should this be moved to aws.py? See https://github.com/edx-solutions/edx-platform/blob/master/lms/envs/aws.py#L489 for reference
There was a problem hiding this comment.
@mattdrayer I'm not sure if aws.py is the right place for FEATURES. I can not see any FEATURES defined in aws.py
1c32731 to
2821bc4
Compare
There was a problem hiding this comment.
Is this supposed to be True? The comment says false.
Same with other setting.
eea9c47 to
f646873
Compare
There was a problem hiding this comment.
I've been learning a bit more about bok choy -- the general approach is to locate these selector queries (either css or xpath) in the Page object, and then expose methods for the Test object to invoke.
So in this particular case we should move settings_detail.q() into SettingsPage, expose via a method (say, get_prerequisite_course_field), and then self.assertTrue(prerequisite_course_field.present) within test_page_has_prerequisite_field
b964a40 to
f890864
Compare
There was a problem hiding this comment.
(NIT) could you change this to .tip (nested within .prerequisites)? we try to avoid element selectors in our SASS.
edit: I realized that you might be referring to the "Requirements" header here, and not the .tip message. It would still be great to use something like .course-details-header {} instead of p {} here.
|
Also, as a quick note for reviewers it might be helpful to clarify that this is a PR to merge in soft-prerequisites (adds messaging to course about pages on openedX) but doesn't yet limit access to courseware. That hard gating/messaging will be added in a follow-up PR. |
|
@srpearce - Hi Sylvia. Would you mind reviewing the documentation/ui impact of this PR? Here is a link to the impact to an example about page (Note- only openedX pages, not drupal pages) http://milestones.m.sandbox.edx.org/courses/edX/DemoX/Demo_Course/about And here is a link to the Studio changes: |
f890864 to
47d9e02
Compare
|
@marcotuts made UX changes as you suggested. |
There was a problem hiding this comment.
You should already have access to %cont-truncated via _mixins.scss in common, so this should work without the additional rule. If not, I would use the existing text-trunctated above it. thanks, and sorry for the confusion.
There was a problem hiding this comment.
_mixins.scss did not have %cont-truncated. Removed mine and using %text-trunctated instead.
|
@marcotuts Hey - was just about to get started with this but the sandbox links aren't working. Help? |
47d9e02 to
0d94db5
Compare
|
@asadiqbal08 is going to jump onto this PR and lend us a hand -- thanks! |
There was a problem hiding this comment.
@marcotuts -- having a hard time determining where to locate this new block of code when rebasing on master -- can you take a look?
There was a problem hiding this comment.
@caesar2164 - Hi! Nov 4 you merged in a PR that removed the prerequisites section fro m the about pages, possibly because it wasn't used, but I wanted to confirm from you what this refactor/feature was focused on : edx@54b7a57
The team is hoping to add support for soft prerequisites (messaging on course about pages) through this PR, soon to be followed up by gated course prerequisites (which would expose messaging on the LMS dashboard and prevent course access until an author has completed prerequisites.
Could you describe your changes and any suggestions you have for the team on adding in prerequisites again? Thanks!
edit - merge time frame was wrong. updated.
There was a problem hiding this comment.
@marcotuts - I only removed styling that made prerequisites different than the rest of the items in the sidebar... I think you should connect this to the preexisting "prerequisites" conditional. See my other comment.
1fa2c2f to
d8d1ae2
Compare
|
FYI, I have rebased this PR onto the latest master revision -- let's see how the CI run goes |
|
Okay, all tests are passing post-rebase -- @ziafazal @marcotuts @smagoun FYI |
|
@chrisndodge test failures. |
|
@cahrens hrm... not sure what happened there. I ran the full suite of LMS tests locally, but apparently it didn't run them all (is there a side effect to using --fasttest?!?). It seemed like it did complete very quickly... :-) Give this a chance to run through a Jenkins pass.... |
|
Merge conflicts now. Content Libraries merged to master, so I can imagine a lot of rebasing going on. |
|
Did someone verify that the course catalog access is working properly? |
|
@cahrens @ziafazal @bradenmacdonald There's two merge conflicts, one of which is trivial. However, there is a merge conflict (due to content libraries landing into master) on cms/djangoapps/contentstore/course.py which I don't feel confident to resolve myself since I'm not one of the authors. @ziafazal can you try to resolve and tag @bradenmacdonald if you have any questions about the conflict resolution? Thanks |
41fa38e to
2808539
Compare
|
@chrisndodge @cahrens @bradenmacdonald merged with master. |
|
@ziafazal What do you mean 'merged with master'? It doesn't look like this PR has been merged on GitHub. Also there are still quite a few PyLint and PEP8 violations that need to be fixed as they are breaking the build. |
|
@ziafazal I pulled the latest changes from master and I'm not seeing any of these commits there yet. |
|
@ziafazal In case you're not familiar with the Jenkins output, this console log shows the PEP8/PyLint failures: https://jenkins.testeng.edx.org/job/edx-all-tests-auto-pr/21726/SHARD=1,TEST_SUITE=quality/console |
|
@andy-armstrong sorry I should have said "Rebased with master" and resolved conflicts. Let me fix the quality errors. |
|
There is a bok choy failure in a test related to this PR. |
|
Yes, I'm trying to get a hand from the testeng folks on this error -- it On Wed, Jan 14, 2015 at 10:12 AM, Christina Roberts <
Matthew Drayer |
|
👍 after the following:
|
242a8df to
6733d04
Compare
6733d04 to
ca3d84a
Compare
|
All validation suites passed, merging feature |
…ia-milestones-app Ziafazal/add pre requisite course via milestones app
10 participants
@mattdrayer added pre-requisite feature via milestones app. It would show a drop down for course authors to select from list available courses as a pre-requisite course in "Settings & Details" page. Inside lms it would show the appropriate messaging under side bar of course about page.