Skip to content

Commit

Permalink
docs: clarify comment that I had added earlier
Browse files Browse the repository at this point in the history
  • Loading branch information
@bradenmacdonald
bradenmacdonald committed May 31, 2023
1 parent dac1c5a commit d44ca41
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion xmodule/x_module.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ def location(self):
def location(self, value):
assert isinstance(value, UsageKey)
self.scope_ids = self.scope_ids._replace(
def_id=value, # FIXME: this seems wrong... a UsageKey is not a definition key.
def_id=value, # Note: assigning a UsageKey as def_id is OK in old mongo / import system but wrong in split
usage_id=value,
)

Expand Down

4 comments on commit d44ca41

@kdmccormick
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @bradenmacdonald , I ran across this comment while looking into some followup on openedx/XBlock#690.

The way I read this comment is that if someone calls block.location = ... on a block from SplitMongo, then we will be a "wrong" situation. So, is this only called for blocks from the OldMongo stub store, which IIRC are only CourseBlocks at this point?

@bradenmacdonald
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the whole reason "split" is called "split" is because definition data is separate from usage data. And they have different types of identifiers. So it's definitely wrong to assign some value to both scope IDs in split mongo. (In Learning Core, who knows - we might get rid of the definition IDs).

But I don't even know if/when this setter would ever be used. Normally you set an XBlock's scope_ids in the constructor and they never change for the life of the block. The only thing that can change is the user_id can be modified when the block is (re)bound to a user. Personally, I'd want to just remove this @location.setter function entirely, and see if anything breaks. "Old Mongo" might still be used for reading some legacy courses which are still in that format, but I'm not sure if this code path is involved in that or not.

@bradenmacdonald
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kdmccormick ^

@kdmccormick
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd want to just remove this @location.setter function entirely, and see if anything breaks.

Sounds good to me 👍🏻

Please sign in to comment.