More xblock.save() calls to handle fields set during inits by dmitchell · Pull Request #505 · openedx/openedx-platform

dmitchell · 2013-07-25T21:11:16Z

and other possible side effecting calls

@cpennington @chrisndodge Please review.

I need to think of tests which will fail w/o these changes and then prove they pass, but hopefully otherwise this is done.

chrisndodge · 2013-07-26T01:57:30Z

IMO, does seem like overkill. Can you explain why you think you might need this?

Only an abundance of caution. The problem is that init on some of our
descriptors set fields. I think I ensured that get_item will call .save()
but preview had an odd pattern which I vacillated on.

On Thu, Jul 25, 2013 at 9:57 PM, chrisndodge notifications@github.comwrote:

In cms/djangoapps/contentstore/views/preview.py:

@@ -43,6 +43,8 @@ def preview_dispatch(request, preview_id, location, dispatch=None):
"""

descriptor = modulestore().get_item(location)

decache any pending field settings -- this one may be overkill

descriptor.save()

IMO, does seem like overkill. Can you explain why you think you might need
this?

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/505/files#r5414625
.

I'm happy to remove that one. I put the comment there particularly to see
if anyone thought it was necessary to keep.

On Fri, Jul 26, 2013 at 8:51 AM, Don-edX Mitchell dmitchell@edx.org wrote:

Only an abundance of caution. The problem is that init on some of our
descriptors set fields. I think I ensured that get_item will call .save()
but preview had an odd pattern which I vacillated on.

On Thu, Jul 25, 2013 at 9:57 PM, chrisndodge notifications@github.comwrote:

In cms/djangoapps/contentstore/views/preview.py:

@@ -43,6 +43,8 @@ def preview_dispatch(request, preview_id, location, dispatch=None):
"""

descriptor = modulestore().get_item(location)

decache any pending field settings -- this one may be overkill

descriptor.save()

IMO, does seem like overkill. Can you explain why you think you might
need this?

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/505/files#r5414625
.

Yeah, given that load_item has the call to .save(), I don't think we should add it here.

chrisndodge · 2013-07-28T21:29:47Z

I can't VPN into jenkins, any ideas why the build is failing?

dmitchell · 2013-07-29T12:54:34Z

@dianakhuang @sarina Here's the PR I put in for the missing save calls.

dmitchell · 2013-07-29T12:55:50Z

Build failure was git or gcli. That is, never even made it to the build.

cpennington · 2013-07-29T12:58:46Z

We should call .save on parent_xblock as well, since its children have been edited.

However, I think that xblock_from_json is probably the thing that should be responsible for calling .save() on the new_block

dmitchell · 2013-07-29T13:16:25Z

All comments verified and addressed, thank you all. Still no unit tests, but can I get approval to merge?

cpennington · 2013-07-29T13:22:56Z

Have you/could you verify that it fixes the issue w/ saw last week on edge?

dmitchell · 2013-07-29T13:38:36Z

I never saw the problem on any env other than edge. Setting up for
reproduction would be onerous. I'd have to roll back my system to before
Peter Fogg's and possibly the xblock save() changes, create a course w/
video pointing to something other than Lyla, and then roll forward. Knowing
my luck, I wouldn't be able to reproduce the bug; so, I wouldn't be able to
verify the fix. As I didn't cause the bug, that seems more than heroic
counter measures and a skill set better suited to a QA engineer.

On Mon, Jul 29, 2013 at 9:23 AM, Calen Pennington
notifications@github.comwrote:

Have you/could you verify that it fixes the issue w/ saw last week on edge?

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

sarina · 2013-07-29T14:10:11Z

I can repro the issue on edge (it's still on master). Checking out this branch appears to fix the issue (I see Gregory Nagy's beautiful floating head once again).

dmitchell · 2013-07-29T14:20:51Z

Sarina, You're my star. Thank you.

Peter's writing a unit test (create video descriptor and then access field
via own_metadata() rather than field accessor to go around ModelType
retrieval logic).

On Mon, Jul 29, 2013 at 10:10 AM, Sarina Canelake
notifications@github.comwrote:

I can repro the issue on edge (it's still on master). Checking out this
branch appears to fix the issue (I see Gregory Nagy's beautiful floating
head once again).

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

chrisndodge · 2013-07-29T15:16:52Z

These edits (PEP8?) appear unrelated to the bug fix. I'm fine with it, but it seems extraneous.

Well, it had some changes which I removed. I thought about taking it out but thought it didn't matter.

chrisndodge · 2013-07-29T15:17:15Z

+1. Just one comment that a changed file appears to be unrelated to the bug fix.

sarina · 2013-07-29T15:33:17Z

Two things - what if parent_xblock is None? (this line will probably say, "Nonetype object has no attribute 'save'"

Second thing - until I'm done working on https://edx-wiki.atlassian.net/browse/LMS-769 , mutable types won't actually be persisted if you only call things like append. You'll need to do something like children.append(new_block); parent_xblock.children = children so that you actually hit a __set__ call, so that the save will actually persist the new list.

That's what happens when I just listen to comments and don't think. Thank you.

sarina · 2013-07-29T16:27:27Z

Looks good to me. Pretty sure this will build properly but may be worth running tests locally before merging, since Jenkins is down at the moment and we want to be safe!

dmitchell · 2013-07-29T16:34:32Z

I ran the tests.

On Mon, Jul 29, 2013 at 12:27 PM, Sarina Canelake
notifications@github.comwrote:

Looks good to me. Pretty sure this will build properly but may be worth
running tests locally before merging, since Jenkins is down at the moment
and we want to be safe!

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

More xblock.save() calls to handle fields set during inits

Feature/tomg/fall design

[YONK-146] Fix GP v1 notifications

…x-favicon-path Fix favicon display

…ponent-id-overflow Hide overflow of component ID in studio

PR openedx#505 mistakenly omitted the microfrontend services from the new `DEFAULT_SERVICES` list, which determines what is brought up upon `make dev.up`.

chrisndodge reviewed Jul 26, 2013
View reviewed changes

cpennington reviewed Jul 29, 2013
View reviewed changes

More xblock.save() calls to handle fields set during inits

d1ed6fb

chrisndodge reviewed Jul 29, 2013
View reviewed changes

sarina reviewed Jul 29, 2013
View reviewed changes

Ensure parent update of children decaches correctly

393aa1d

dmitchell added a commit that referenced this pull request Jul 29, 2013

Merge pull request #505 from edx/dhm/xblock_save

0578175

More xblock.save() calls to handle fields set during inits

dmitchell merged commit 0578175 into master Jul 29, 2013

cpennington deleted the dhm/xblock_save branch July 29, 2013 16:37

peter-fogg mentioned this pull request Jul 29, 2013

Add (currently failing) test case around saving metadata. #514

Merged

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014

Merge pull request openedx#505 from MITx/feature/tomg/fall-design

473fd9b

Feature/tomg/fall design

bradenmacdonald mentioned this pull request Jun 23, 2015

Bump python-social-auth to upstream's latest master #8599

Merged

Kelketek referenced this pull request in open-craft/openedx-platform Sep 17, 2015

Merge pull request #505 from edx-solutions/group_project_v2_cypress

90a9441

[YONK-146] Fix GP v1 notifications

hachiyanagi-ks added a commit to nttks/edx-platform that referenced this pull request Nov 20, 2015

Merge pull request openedx#505 from hachiyanagi-ks/develop/cypress/fi…

5d8abbc

…x-favicon-path Fix favicon display

diegomillan pushed a commit to eduNEXT/edx-platform that referenced this pull request Sep 14, 2016

Merge pull request openedx#505 from Stanford-Online/giulio/studio-com…

e0093ba

…ponent-id-overflow Hide overflow of component ID in studio

Uh oh!

Conversation

dmitchell commented Jul 25, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

decache any pending field settings -- this one may be overkill

Uh oh!

Choose a reason for hiding this comment

decache any pending field settings -- this one may be overkill

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisndodge commented Jul 28, 2013

Uh oh!

dmitchell commented Jul 29, 2013

Uh oh!

dmitchell commented Jul 29, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitchell commented Jul 29, 2013

Uh oh!

cpennington commented Jul 29, 2013

Uh oh!

dmitchell commented Jul 29, 2013

Uh oh!

sarina commented Jul 29, 2013

Uh oh!

dmitchell commented Jul 29, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisndodge commented Jul 29, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarina commented Jul 29, 2013

Uh oh!

dmitchell commented Jul 29, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants