Skip to content

prevent failure when module does not have a data field - #1682

Merged
mulby merged 1 commit into
openedx:masterfrom
mulby:gabe/fix-get-items-without-data
Jan 3, 2014
Merged

prevent failure when module does not have a data field#1682
mulby merged 1 commit into
openedx:masterfrom
mulby:gabe/fix-get-items-without-data

Conversation

@mulby

@mulby mulby commented Nov 15, 2013

Copy link
Copy Markdown
Contributor

This raised an AttributeError when I tried to retrieve a vertical or a course using the REST API.

Reviewers: @dmitchell, @singingwolfboy

@mulby

mulby commented Nov 15, 2013

Copy link
Copy Markdown
Contributor Author

Is there already a bank of tests for the REST API that I could add a test to?

@mulby

mulby commented Nov 18, 2013

Copy link
Copy Markdown
Contributor Author

@dmitchell @singingwolfboy - bump

@dmitchell

Copy link
Copy Markdown
Contributor

I hope to get to this today. Getting internet at home and going to doctors
shortened my work hours since Friday (I thought I'd have internet over the
weekend to catch up, but installation failed. All set now though!)

On Mon, Nov 18, 2013 at 11:49 AM, Gabe Mulley notifications@github.comwrote:

@dmitchell https://github.com/dmitchell @singingwolfboyhttps://github.com/singingwolfboy- bump


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/1682#issuecomment-28715142
.

@singingwolfboy

Copy link
Copy Markdown
Contributor

This seems reasonable to me. I don't know anything about a bank of REST API tests, unfortunately -- but @dmitchell might know.

@dmitchell

Copy link
Copy Markdown
Contributor

👍

as to a bank of tests, we've been changing existing tests to the new api or making one test file per view file. It would be great if you could add a test to cms.djangoapps.contentstore.tests.test_item.ItemTest (I note that I neglected to do so.)

@sarina

sarina commented Dec 3, 2013

Copy link
Copy Markdown
Contributor

Hey what's going on with this PR? It's been untouched for 14 days.

@dmitchell

Copy link
Copy Markdown
Contributor

I think Gabe should merge it.

@mulby

mulby commented Dec 3, 2013

Copy link
Copy Markdown
Contributor Author

I've been 100% focused on the datajam lately, originally needed this for it, turns out we don't really, so I shifted my focus elsewhere. I was planning on adding tests for it after we get the datajam stuff squared away and then merging it. I can merge now, though, if that's helpful.

@sarina

sarina commented Dec 3, 2013

Copy link
Copy Markdown
Contributor

I'm just trying to clear out old PRs. Do what makes sense for you.

@singingwolfboy

Copy link
Copy Markdown
Contributor

@mulby Now that the datajam is done, are you ready to merge this?

@mulby

mulby commented Jan 3, 2014

Copy link
Copy Markdown
Contributor Author

Alright, I've added a test. Didn't invest too much in surrounding coverage, however, this test does cover the change. When/if this passes the CI tests, I'll merge it in to master.

mulby added a commit that referenced this pull request Jan 3, 2014
prevent failure when module does not have a data field
@mulby
mulby merged commit 34a340a into openedx:master Jan 3, 2014
@mulby
mulby deleted the gabe/fix-get-items-without-data branch January 3, 2014 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants