Origin/backend/enforce story translation stage logic #99

jennifer-lu · 2021-09-20T02:49:31Z

Notion ticket link

Enforce story translation stage logic

Implementation description

Raises an exception if a user is not authorized to leave comments or update story translation contents
The above is based on the story translation stage and whether the user is a translator or a reviewer
For comments (see create_comment in comment_service.py): if the stage is TRANSLATE, translators can leave comments and reviewers cannot; if the stage is REVIEW, reviewers can leave comments and translators cannot; if the stage is PUBLISH, no comments can be left
For contents (see update_story_translation_contents in story_service.py): contents can be changed only if the stage is TRANSLATE

Steps to test

Setup Altair as translator (e.g. carlsagan)
Run the following, no exception should come up
mutation { createComment (commentData: { storyTranslationContentId: 55, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, exception should be 'You are not authorized to leave comments on this story.'
mutation { createComment (commentData: { storyTranslationContentId: 25, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, no exception should come up
mutation { updateStoryTranslationContents (storyTranslationContents: { id: 55, translationContent: "hi", }) { story { id translationContent } } }
Setup Altair as reviewer (e.g. dwightdeisenhower)
Run the following, exception should be 'Comments cannot be left by reviewers while the story is being translated.'
mutation { createComment (commentData: { storyTranslationContentId: 55, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, exception should be 'You are not authorized to leave comments on this story.'
mutation { createComment (commentData: { storyTranslationContentId: 25, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
In the mysql database, update story translation with id=6 to state 'REVIEW' update story_translations set stage = 'REVIEW' where story_id = 6;
Setup Altair as translator
Run the following, exception should be 'Comments cannot be left by translators while the story is being reviewed.'
mutation { createComment (commentData: { storyTranslationContentId: 55, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, exception should be 'You are not authorized to leave comments on this story.'
mutation { createComment (commentData: { storyTranslationContentId: 25, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, exception should be 'Story translation contents cannot be changed while the story is being reviewed.'
mutation { updateStoryTranslationContents (storyTranslationContents: { id: 55, translationContent: "hi", }) { story { id translationContent } } }
Setup Altair as reviewer
Run the following, no exception should come up
mutation { createComment (commentData: { storyTranslationContentId: 55, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, exception should be 'You are not authorized to leave comments on this story.'
mutation { createComment (commentData: { storyTranslationContentId: 25, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Setup Altair as translator
Run the following, exception should be 'Comments cannot be left after the story has been published.'
mutation { createComment (commentData: { storyTranslationContentId: 55, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, exception should be 'You are not authorized to leave comments on this story.'
mutation { createComment (commentData: { storyTranslationContentId: 25, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, exception should be 'Story translation contents cannot be changed after the story has been published.'
mutation { updateStoryTranslationContents (storyTranslationContents: { id: 55, translationContent: "hi", }) { story { id translationContent } } }
Setup Altair as reviewer
Run the following, exception should be 'Comments cannot be left after the story has been published.'
mutation { createComment (commentData: { storyTranslationContentId: 55, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }
Run the following, exception should be 'You are not authorized to leave comments on this story.'
mutation { createComment (commentData: { storyTranslationContentId: 25, content: "", }) { ok comment { id storyTranslationContentId userId commentIndex time resolved content } } }

What should reviewers focus on?

Logic is correct and works as expected (see last two points of implementation)
Exception messages are correct

Checklist

My PR name is descriptive and in imperative tense
My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
For backend changes, I have run the appropriate linters: docker exec -it planet-read_py-backend_1 /bin/bash -c "black . && isort --profile black ." and I have generated new migrations: flask db migrate -m "<your message>"
I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

gaoxk

nice! everything worked for me while testing. Rebase onto main branch, work on the feedback, and let me know when it's ready for re-review!

gaoxk · 2021-09-21T23:55:15Z

backend/python/app/services/implementations/comment_service.py

-                    Comment.story_translation_content_id
-                    == comment["story_translation_content_id"]
+        user_id = get_user_id_from_request()
+        new_comment = Comment(**comment)


20-37 should be wrapped in a try statement (see how it was previously wrapped)

gaoxk · 2021-09-21T23:57:09Z

backend/python/app/services/implementations/comment_service.py

+        story_translation_translator_id = story_translation.translator_id
+        story_translation_reviewer_id = story_translation.reviewer_id
+
+        is_translator = user_id == str(story_translation_translator_id)


rather than wrap str() around story_translation_translator_id and story_translation_reviewer_id, can you wrap get_user_id_from_request() with int() on line 20 and leave a comment to remove it when get_user_id_from_request() is updated to return ints? example comment

gaoxk · 2021-09-22T00:00:57Z

backend/python/app/services/implementations/comment_service.py

+        new_comment = Comment(**comment)
+        new_comment.user_id = user_id
+
+        story_translation_content = StoryTranslationContent.query.filter_by(


nice! this works, but getting story_translation_content and getting story_translation on line 29 are two database queries. Can you try using only 1 database query? You'll need to join StoryTranslation and StoryTranslationContent together.

gaoxk · 2021-09-22T00:01:50Z

backend/python/app/services/implementations/story_service.py

-            self.logger.error(
-                "Failed to update story translation content. Reason = {reason}".format(
-                    reason=(reason if reason else str(error))
+        story_translation_content = StoryTranslationContent.query.filter_by(


ditto comment about wrapping this in a try statement. You should always wrap any database interactions with try statements, because they could always fall through if the database has issues!

gaoxk · 2021-09-22T00:02:01Z

seed_db.sh

@@ -1,45 +1,45 @@
 # ***** How to Use: *****


is this a result of the linter?

gaoxk

Nice! feel free to merge after addressing nits. let me know if you have any questions about the nit
(((A relevant stackoverflow thread you might find interesting)))

gaoxk · 2021-09-23T02:22:43Z

backend/python/app/services/implementations/comment_service.py


-            max = (
-                db.session.query(func.max(Comment.comment_index))
+            story_translation = (


nice query! nit: I believe db.session.query(StoryTranslation).join(.... could be rewritten as StoryTranslation.query.join(.... I think the latter option is more clear

gaoxk · 2021-09-23T02:23:02Z

backend/python/app/services/implementations/story_service.py

-            db.session.bulk_update_mappings(
-                StoryTranslationContent, story_translation_contents
+            story_translation = (
+                db.session.query(StoryTranslation)


ditto comment

gaoxk · 2021-09-23T02:23:05Z

backend/python/app/services/implementations/story_service.py

-            story_translation = StoryTranslationContent.query.filter_by(
-                id=story_translation_content.id
-            ).first()
+        story_translation = StoryTranslationContent.query.filter_by(


ditto comment

Can I leave this one because it's in the deprecated function?

jennifer-lu requested a review from gaoxk September 20, 2021 02:49

gaoxk requested changes Sep 22, 2021

View reviewed changes

jennifer-lu added 2 commits September 21, 2021 20:53

Enforce story translation stage logic

fa71e7a

Address enforce story translation stage logic PR comments

1dec0b1

jennifer-lu force-pushed the origin/backend/enforce-story-translation-stage-logic branch from 96d0a66 to 1dec0b1 Compare September 22, 2021 02:57

gaoxk approved these changes Sep 23, 2021

View reviewed changes

fixup! Address enforce story translation stage logic PR comments

c1099ff

jennifer-lu merged commit 0dc9b88 into main Sep 23, 2021

jennifer-lu deleted the origin/backend/enforce-story-translation-stage-logic branch September 23, 2021 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Origin/backend/enforce story translation stage logic #99

Origin/backend/enforce story translation stage logic #99

jennifer-lu commented Sep 20, 2021

gaoxk left a comment

gaoxk Sep 21, 2021

gaoxk Sep 21, 2021

gaoxk Sep 22, 2021

gaoxk Sep 22, 2021

gaoxk Sep 22, 2021

gaoxk left a comment

gaoxk Sep 23, 2021

gaoxk Sep 23, 2021

gaoxk Sep 23, 2021

jennifer-lu Sep 23, 2021 •

edited

Loading

Origin/backend/enforce story translation stage logic #99

Origin/backend/enforce story translation stage logic #99

Conversation

jennifer-lu commented Sep 20, 2021

Notion ticket link

Implementation description

Steps to test

What should reviewers focus on?

Checklist

gaoxk 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

gaoxk 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

jennifer-lu Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

jennifer-lu Sep 23, 2021 •

edited

Loading