Set definition_locator on in-memory xblocks

[dmitchell]

@cpennington please review

[cpennington]

a) You don't need to check NoneType at the end, because you check None at the beginning.
b) Can we just make sure that definition_locators are never None?

[dmitchell]

Yeah, I didn't think I needed the test (but it's testing the internals of the thing checked as None earlier; so, it's not redundant.) The other place in the file used the less lenient test. I changed this. I think it still needs to check the locator b/c I don't trust all xblock constructors to add it (e.g., when migrating from one modulestore to another)

[cpennington]

Looks reasonable. Are there any new tests needed to cover this case?

[dmitchell]

Well, since it broke 4 in test_crud, I think it's covered :-) I could add
explicit test which check that the definition_id is indeed a LocalId if you
think that would be useful.

On Thu, Jan 9, 2014 at 10:27 AM, Calen Pennington
notifications@github.comwrote:

Looks reasonable. Are there any new tests needed to cover this case?

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

[cpennington]

Yeah, I think it might be helpful to cement that behavior, if we're going
to be depending on it.

-Cale

On Thu, Jan 9, 2014 at 10:32 AM, Don Mitchell notifications@github.comwrote:

Well, since it broke 4 in test_crud, I think it's covered :-) I could add
explicit test which check that the definition_id is indeed a LocalId if
you
think that would be useful.

On Thu, Jan 9, 2014 at 10:27 AM, Calen Pennington
notifications@github.comwrote:

Looks reasonable. Are there any new tests needed to cover this case?

—
Reply to this email directly or view it on GitHub<
https://github.com/edx/edx-platform/pull/2117#issuecomment-31942101>
.

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

[cpennington]

👍