Add backward linkage of story to the topic it belongs. #6855

DubeySandeep · 2019-06-06T20:13:11Z

Explanation

Add backward linkage of story to the topic it belongs.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

DubeySandeep · 2019-06-06T20:18:43Z

core/domain/story_domain.py

+                'Expected belongs_to_topic should be a string, received: %s' % (
+                    self.belongs_to_topic))
+
+        if len(self.belongs_to_topic) != 12:


@seanlip, Can you give your suggestion on these questions:
(belongs_to_topic = the topic id to which the story belongs)

Do we need to expose belongs_to_topic value to domain layer as we don't expect users to change this value?

If yes to the above question, then do we have to validate the topic_id? (note we do not validate the story_id)

@aks681, is there any specific reason for using created_on and last_updated in the domain layer?

"belongs_to_topic" sounds like a boolean. Maybe corresponding_topic_id or something like that.

Yes, expose to domain layer. Domain layer is where the logic is.

No need to validate topic ID.

I remember using collections as a reference when creating the new structure models and it had those 2 fields. So, I added that here as well.
I don't think you need to validate the existence of the topic, though you can make sure it's a 12 character string. I think the former is usually done in the controller layer. What do you think?

Yeah, I've done the validation check for the existence of topic in the service layer. Removed the simple validation check for the topic id from the domain layer.

seanlip

Thanks @DubeySandeep! I took a quick glance at the overall structure, but woudl defer the more thorough review to @aks681.

One thing I'm confused about is -- shouldn't we have some code that handles stuff like "story added to topic", "story deleted from topic" (and adjusts the topic linkages accordingly)? I can't see where that code lives, but perhaps I'm missing it.

seanlip · 2019-06-06T23:12:42Z

utils.py

@@ -487,7 +487,10 @@ def generate_random_string(length):
    Returns:
        str. Random string of specified length.
    """
-    return base64.urlsafe_b64encode(os.urandom(length))
+    random_string = ''


Oh, hm, interesting. Was the old function creating strings that were too short? Based on reading the python docs, I'm not sure I understand why that would happen... what's an example?

I wasn't sure about the case where we can have len(string) < length, but for me it was len(string) > length.

I read to docs in detail and I think it will only generate len(String) > length. I've made the changes!

Oh, I see, urandom() creates a string of length 12 but urlsafe_b64encode might extend that. Thanks for clarifying!

We could alternatively just take the first "length" characters...

aks681

Just some minor comments, but looks good!
One question though, would this be required in the frontend?

aks681 · 2019-06-10T19:35:41Z

core/domain/story_domain.py

+                'Expected belongs_to_topic should be a string, received: %s' % (
+                    self.belongs_to_topic))
+
+        if len(self.belongs_to_topic) != 12:


I remember using collections as a reference when creating the new structure models and it had those 2 fields. So, I added that here as well.
I don't think you need to validate the existence of the topic, though you can make sure it's a 12 character string. I think the former is usually done in the controller layer. What do you think?

aks681 · 2019-06-10T19:39:34Z

core/domain/story_services.py

+
+    if story.id not in topic.canonical_story_ids + topic.additional_story_ids:
+        raise Exception(
+            'Expected story to belongs to the topic %s, but it is '


..belong to the topic.

aks681 · 2019-06-10T19:40:24Z

core/domain/story_services.py

+    if story.id not in topic.canonical_story_ids + topic.additional_story_ids:
+        raise Exception(
+            'Expected story to belongs to the topic %s, but it is '
+            'neither a part of canonical story or the addition story of '


'the canonical stories or the additional stories'

aks681 · 2019-06-10T19:41:31Z

core/storage/story/gae_models.py

@@ -58,6 +58,8 @@ class StoryModel(base_models.VersionedModel):
    # The schema version for the story_contents.
    story_contents_schema_version = (
        ndb.IntegerProperty(required=True, indexed=True))
+    # The topic id to which the story belongs.
+    belongs_to_topic = ndb.StringProperty(indexed=False, required=False)


@seanlip. Will this cause issues with existing stories in the datastore?

No, I think it's fine. They'll just be set to None.

But I think indexed should be True. And once we've populated the story models on the test server correctly, we should make this required=True (is that right, @DubeySandeep? If so, we should add a TODO to do this.)

For now, I've changed both of the value to True once we will deploy the new branch to testserver we can add topic_id manually before start testing. I'll add an email (with the story id and corresponding topic id) to Sean for the same (for tracking).

…o-story

aks681

LGTM!
Though, I'd suggest adding at least the simple basestring validation check to story_domain, as all other fields have their checks there also.
Also note, failing test.

…o-story

DubeySandeep · 2019-06-13T14:09:38Z

@aks681, I've fixed the tests and added validation check to story_domain.

@nithusha21, Can you please review this PR as a codeowner?

codecov · 2019-06-13T16:38:21Z

Codecov Report

Merging #6855 into develop will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #6855      +/-   ##
===========================================
+ Coverage    95.68%   95.68%   +<.01%     
===========================================
  Files          371      371              
  Lines        51360    51429      +69     
===========================================
+ Hits         49143    49212      +69     
  Misses        2217     2217

Impacted Files	Coverage Δ
core/storage/story/gae_models_test.py	`100% <ø> (ø)`	⬆️
core/controllers/topic_editor.py	`84.43% <ø> (ø)`	⬆️
core/controllers/story_viewer_test.py	`100% <100%> (ø)`	⬆️
core/tests/test_utils.py	`95.72% <100%> (ø)`	⬆️
utils_test.py	`100% <100%> (ø)`	⬆️
core/domain/story_services_test.py	`100% <100%> (ø)`	⬆️
core/storage/story/gae_models.py	`100% <100%> (ø)`	⬆️
core/controllers/topic_viewer_test.py	`100% <100%> (ø)`	⬆️
core/domain/story_services.py	`83.94% <100%> (+0.38%)`	⬆️
core/controllers/story_editor_test.py	`100% <100%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7074567...fc64194. Read the comment docs.

nithusha21

Just a couple of nits in the docstring. LGTM as a codeowner.

nithusha21 · 2019-06-13T19:17:40Z

core/tests/test_utils.py

@@ -1281,13 +1281,16 @@ def save_new_story(
            description: str. The high level description of the story.
            notes: str. A set of notes, that describe the characters,
                main storyline, and setting.
+            corresponding_topic_id: str. The topic id to which the story belongs


Suggested change

corresponding_topic_id: str. The topic id to which the story belongs

corresponding_topic_id: str. The id of the topic to which the story belongs.

nithusha21 · 2019-06-13T19:17:53Z

core/tests/test_utils.py

@@ -1281,13 +1281,16 @@ def save_new_story(
            description: str. The high level description of the story.
            notes: str. A set of notes, that describe the characters,
                main storyline, and setting.
+            corresponding_topic_id: str. The topic id to which the story belongs
+                to.


Remove the second "to"

nithusha21 · 2019-06-13T19:18:05Z

core/tests/test_utils.py

@@ -1317,6 +1321,8 @@ def save_new_story_with_story_contents_schema_v1(
            description: str. The high level description of the story.
            notes: str. A set of notes, that describe the characters,
                main storyline, and setting.
+            corresponding_topic_id: str. The topic id to which the story belongs


…o-story

DubeySandeep · 2019-06-13T19:37:03Z

@seanlip & @apb7, can you please review this PR as a codeowner?

seanlip

No comments, this looks excellent IMO. Thanks @DubeySandeep (and @aks681 @nithusha21 for reviewing)!

Add backward linkage of story to the topic.

79231f8

DubeySandeep added the CHANGELOG: New structures label Jun 6, 2019

DubeySandeep requested review from aks681, apb7, nithusha21 and seanlip as code owners June 6, 2019 20:13

DubeySandeep commented Jun 6, 2019

View reviewed changes

DubeySandeep assigned aks681 Jun 6, 2019

seanlip requested changes Jun 6, 2019

View reviewed changes

Change random string generator function.

d5f5d05

aks681 reviewed Jun 10, 2019

View reviewed changes

DubeySandeep added 3 commits June 11, 2019 14:28

Merge branch 'develop' of https://github.com/oppia/oppia into topic-t…

049334f

…o-story

Fix backend test issue.

9eb8743

Change belong_to_topic to corresponding_topic_id.

502a502

aks681 approved these changes Jun 12, 2019

View reviewed changes

aks681 assigned DubeySandeep and unassigned aks681 Jun 12, 2019

DubeySandeep added 3 commits June 13, 2019 18:19

Fix test issues and address review comments.

0386a36

Merge branch 'develop' of https://github.com/oppia/oppia into topic-t…

040561c

…o-story

Fix lint issues.

70f1b1c

DubeySandeep assigned nithusha21 and unassigned DubeySandeep Jun 13, 2019

DubeySandeep assigned seanlip Jun 13, 2019

aks681 assigned apb7 Jun 13, 2019

nithusha21 approved these changes Jun 13, 2019

View reviewed changes

nithusha21 removed their assignment Jun 13, 2019

DubeySandeep added 2 commits June 14, 2019 01:02

Adds proper docstring for corresponding_topic_id.

d746146

Merge branch 'develop' of https://github.com/oppia/oppia into topic-t…

fc64194

…o-story

seanlip approved these changes Jun 14, 2019

View reviewed changes

seanlip merged commit 3069fe6 into oppia:develop Jun 14, 2019

DubeySandeep mentioned this pull request Jun 15, 2019

[Milestone 1.1] Implement review test using question player #6850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add backward linkage of story to the topic it belongs. #6855

Add backward linkage of story to the topic it belongs. #6855

DubeySandeep commented Jun 6, 2019

DubeySandeep Jun 6, 2019 •

edited

Loading

seanlip Jun 6, 2019

aks681 Jun 10, 2019

DubeySandeep Jun 12, 2019

seanlip left a comment

seanlip Jun 6, 2019

DubeySandeep Jun 7, 2019

seanlip Jun 7, 2019

DubeySandeep Jun 12, 2019

aks681 left a comment

aks681 Jun 10, 2019

aks681 Jun 10, 2019

DubeySandeep Jun 12, 2019

aks681 Jun 10, 2019

DubeySandeep Jun 12, 2019

aks681 Jun 10, 2019

seanlip Jun 11, 2019

DubeySandeep Jun 12, 2019

aks681 left a comment •

edited

Loading

DubeySandeep commented Jun 13, 2019

codecov bot commented Jun 13, 2019 •

edited

Loading

nithusha21 left a comment

nithusha21 Jun 13, 2019

DubeySandeep Jun 13, 2019

nithusha21 Jun 13, 2019

DubeySandeep Jun 13, 2019

nithusha21 Jun 13, 2019

DubeySandeep Jun 13, 2019

DubeySandeep commented Jun 13, 2019

seanlip left a comment •

edited

Loading

	corresponding_topic_id: str. The topic id to which the story belongs
	corresponding_topic_id: str. The id of the topic to which the story belongs.

Add backward linkage of story to the topic it belongs. #6855

Add backward linkage of story to the topic it belongs. #6855

Conversation

DubeySandeep commented Jun 6, 2019

Explanation

Checklist

DubeySandeep Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aks681 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aks681 left a comment • edited Loading

Choose a reason for hiding this comment

DubeySandeep commented Jun 13, 2019

codecov bot commented Jun 13, 2019 • edited Loading

Codecov Report

nithusha21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DubeySandeep commented Jun 13, 2019

seanlip left a comment • edited Loading

Choose a reason for hiding this comment

DubeySandeep Jun 6, 2019 •

edited

Loading

aks681 left a comment •

edited

Loading

codecov bot commented Jun 13, 2019 •

edited

Loading

seanlip left a comment •

edited

Loading