Feature/cdodge/allow course run in course create - #510
Conversation
|
@chrisndodge merge conflict (probably changelog file). |
|
@cahrens frances is taking a look at the merge conflict. Can you continue the review? |
There was a problem hiding this comment.
Need to put var before variable (else they are global).
|
@cahrens there are no new BDD tests for failure cases. I updated existing tests for the 'happy path'. |
|
Would like to see BDD spec covering all the error cases. By BDD spec, I mean a wiki page spec with the correct tag so it shows up here: https://edx-wiki.atlassian.net/wiki/display/STU/Studio+BDD+Specs+index?src=search. Extend existing Selenium tests to cover some or all of the error cases (depending on complexity of the setup). |
|
I spent a little time looking at writing Lettuce/BDD specs. In short, I'm not going to be able to do this as I'm behind on other commitments and I need to timebox my time here. So can you two huddle and decide if you want to move ahead with this merge or wait for Lettuce tests for the failure cases. If the latter, then this will have to get re-assigned. |
|
@frrrances this looks solid from my perspective. Nice work. I did abstract out some of the shared rules for primary/secondary actions within forms.scss quickly. Aside from that, we'll just need to sync this up with the "admin rights" branch (https://github.com/edx/edx-platform/pull/470) work that extends (showing/hiding create in-context forms with animations) some of this work. More of a reminder for me/you, depending on what gets merged to master first. 👍 |
|
I feel strongly that we need, at a minimum, a manual BDD spec (wiki page) covering the error cases. See https://edx-wiki.atlassian.net/wiki/display/STU/Studio+BDD+Specs+index?src=search. Maybe @frrrances could help with this? We said in Studio retro that we wanted to get back to requiring this for features. Personally, I still hate this error message: "Either of organization or course number must be unique". |
|
@cahrens yes, that is poor english. I'll change. |
|
@cahrens updated error message. |
|
👍 Once we have the BDD (manual) spec. It should only take someone about 15 minutes to write that up. |
…cify a course_run. Also update unit tests to also provide this 4th parameter. Styling changes to better lay out the form fields are still TBD.
…not the org, course, or run
|
@frrrances and @chrisndodge, thanks for your help. I've done a couple of things:
The latter involved tweaking the related course creation functions in base.js (mostly DOM selectors and HTML class management). I was able to create a course properly locally as well as make sure validation was working. Can you two confirm on your end that this still works as intended and I didn't muck up any tests? @frrrances, also, this form has the styling support to animate its height in. But there are some visual things I want to walk through with you, so the class that triggers/manages that (.animate) on the form's wrapper is removed for now. Let's chat tomorrow quickly about it. Let me know if you have any questions and remember to pull with the rebase in mind. Thanks again. |
|
hmm. interesting. Tests here (Jenkins) are passing but 'rake test_cms' is failing for me locally (some I18N test). Looking more... |
|
@frrrances do you mind trying 'rake test_cms' on your localdev and see if it passes or fails? I'm still getting errors in the internationalization tests. I'm wondering if there is some malformed HTML in a page response. |
|
Waiting for build to complete before merging.... |
|
Tests pass, but master is above PEP8 limit. Will OK'd merging this. |
…ourse-create Feature/cdodge/allow course run in course create
Blog link fix
…ltrics-xblock Update SHA of qualtrics XBlock
5 participants
No description provided.