Remove default broken URL added to new courses #8590
Conversation
|
Thanks for the pull request, @caesar2164! I've created OSPR-662 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. |
|
Somehow I can't believe that removing that image URL broke 468 tests...test this please, Jenkins... |
|
test this please, 3rd time's the charm? |
|
@caesar2164 when you get in maybe try rebasing it to get a clean build |
| @@ -682,7 +682,7 @@ class CourseFields(object): | |||
| ), | |||
| scope=Scope.settings, | |||
| # Ensure that courses imported from XML keep their image | |||
| default="images_course_image.jpg" | |||
| default="" | |||
There was a problem hiding this comment.
The thing is, this will break XML integration so I don't think this is the right way to go. I'm also not sure who's best to ask here.
There was a problem hiding this comment.
Apparently this note was not explicit enough. This is what I think, but I am not on the TNL team. As mentioned below, I am not qualified to really review this PR and someone in this code area needs to weigh in on this change, and whether or not it is the right way to go.
There was a problem hiding this comment.
"XML integration" meaning that courses authored in XML have their course image I think at a specific place in their course structure. I don't quite recall how the XML courses work, but I believe we still have XML courses on production and we should evaluate whether or not this change would make it so that XML courses no longer display their course image. If they don't, would this be a problem?
|
I'm going to triage this to the TNL team to look at. I'm not qualified to be a reviewer on this PR. thanks for understanding! |
4a4c36b to
079c16e
Compare
|
test this please |
7948c10 to
48e867b
Compare
|
test this please |
|
@cahrens I've already run a few builds on this, and I think it needs a rebase. However as I said on the OSPR ticket, I'd like your team to evaluate whether or not we want this PR and if the approach is correct, if we do want it. I'm happy to work with Giulio to get a build passing at that point. |
|
My concern is if this one-line change causes a lot of tests to fail, then this can't be the right approach. |
|
Fair point, although I was having a really hard time last week though getting any build to pass (took 5 builds to get a one line change to lms/envs/devstack.py to pass). Rebasing might make it clearer if the issue is with the code or with where master was last week, @caesar2164 |
|
I created #8798 on master to see what happens. @peter-fogg introduced the course_image setting with the default value in #700. |
|
@sarina I got 153 test failures on the PR I created-- https://build.testeng.edx.org/job/edx-platform-all-tests-pr-flow/11362/. The author needs to do more investigation before TNL will review this. Note that @peter-fogg works here and would be a good person for Giulio to bounce ideas off of. |
|
@cahrens sounds good. Thanks for the input. |
0985a14 to
f18554f
Compare
4a96fa2 to
9d13f84
Compare
b027859 to
61df45b
Compare
New courses created in studio currently have a missing default image filename added by default. By removing this filename at creation enables the default image set in LMS to display: https://github.com/edx/edx-platform/pull/8282
61df45b to
f28a14c
Compare
|
Is this PR still being worked on? |
1 similar comment
|
Is this PR still being worked on? |
New courses created in studio currently have a broken default image added by default.
By removing this URL at creation, it enables the default image set in LMS to display: https://github.com/edx/edx-platform/pull/8282