Don’t return story translations if user already translating/reviewing for language #229

gaoxk · 2021-12-23T05:00:48Z

Notion ticket link

https://www.notion.so/uwblueprintexecs/a658c933d18c48d4b43fd99bda2bd3b4?v=ba22ac93d09040ff8219c60ac7caabdb&p=fc6fd704b0674be19c756b4a1c30ef6c)

Implementation description

swapped out _get_story_translations_user_translating and _get_story_translations_user_reviewing for flexible query builder
added checks to get_stories_available_for_translation and get_story_translations_available_for_review
- if there exists ongoing story translations with same language+role for user, return []
- this could be swapped with an error message that could be displayed, but the designs for that are TBD.
passed in user_id for admin's UserProfile view to relative mutations

Steps to test

Homepage View

Translate

login as planetread+dwightdeisenhower@uwblueprint.org
observe ongoing no English US, but ongoing English UK and German translations.
go to browse stories. set TRANSLATOR role.
Observe that no results show for English UK and German filters.
set language to English US. Notice results. complete flow and verify new (and correct) story translation in table. (note this tests the modified create_translation!)

Review

stay on same user
observe no ongoing review of English UK, but an ongoing English US
go to browse stories. set REVIEWER role.
Observe that no results show for English US.
set language to English UK. Notice results. complete flow and verify it assigned the user correctly. (note this tests the modified assign_user_as_reviewer!)

Admin UserProfile View

erase & reseed db

docker exec -it planet-read_py-backend_1 /bin/bash -c "python -m tools.insert_test_data erase"
docker exec -it planet-read_py-backend_1 /bin/bash -c "python -m tools.insert_test_data"

Translate

go to http://localhost:3000/user/4 . observe ongoing no English US, but ongoing English UK and German translations.
attempt to assign English US. Notice results. complete flow and verify new (and correct) story translation in table.
attempt to assign English UK and German. Notice empty results for all levels

Review

Stay on the same page. observe no ongoing review of English UK, but an ongoing English US
attempt to assign English UK. Notice results for level 3. complete flow and verify it assigned the user correctly.
attempt to assign English US. Notice empty results for all levels

What should reviewers focus on?

does it work?

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 · 2021-12-23T05:01:44Z

backend/python/app/graphql/queries/story_query.py

@@ -51,3 +59,10 @@ def resolve_story_translation_tests(root, info, language, level, stage, story_ti
    return services["story"].get_story_translation_tests(
        user, language, level, stage, story_title
    )
+
+
+def _select_user_id_for_available_translations_query(user_id):


Putting this here because moving it to story_service means adding the user service to the story service. Also, this sticks to the principle of keeping authentication out of the story services. lmk if you can think of a better name tho LOL

what do you think about something like target_user_id instead of searching_user_id? i'm not sure if it's more readable or not

and consequently something like _target_user_id_for_available_translations_query for the helper

I think the verb "select" makes more sense than "target" for the function name, but I will swap searching_user_id for target_user_id !

gaoxk · 2021-12-23T05:02:31Z

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

@@ -173,7 +165,9 @@ def create_translation_test(self, user_id, level, language):
            raise error
        return new_story_translation

-    def get_story_translations_by_user(self, user_id, is_translator, language, level):
+    def get_story_translations_by_user(
+        self, user_id, is_translator=None, language=None, level=None


made this change to make the function header more readable

github-actions · 2021-12-23T05:03:40Z

Visit the preview URL for this PR (updated for commit 4d1d49a):

https://planet-read-uwbp--pr229-fix-dont-return-tran-lhgz5wmi.web.app

_{(expires Fri, 31 Dec 2021 04:36:26 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

Puepis

LGTM!

Puepis · 2021-12-24T03:29:35Z

backend/python/app/graphql/queries/story_query.py

@@ -51,3 +59,10 @@ def resolve_story_translation_tests(root, info, language, level, stage, story_ti
    return services["story"].get_story_translation_tests(
        user, language, level, stage, story_title
    )
+
+
+def _select_user_id_for_available_translations_query(user_id):


what do you think about something like target_user_id instead of searching_user_id? i'm not sure if it's more readable or not

and consequently something like _target_user_id_for_available_translations_query for the helper

Puepis · 2021-12-24T03:33:47Z

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

+
+            stories = (
+                Story.query.filter(Story.level == level)
+                .filter(~Story.translated_languages.contains(language))


just a q: what does the ~Story... syntax mean?

negation! this reads as return story translations where translated_languages does not contain language

gaoxk requested review from lynn-kim and Puepis December 23, 2021 05:00

gaoxk commented Dec 23, 2021

View reviewed changes

Puepis approved these changes Dec 24, 2021

View reviewed changes

Don’t return story trans if language in progress

4d1d49a

gaoxk force-pushed the fix/dont-return-trans branch from a108f5f to 4d1d49a Compare December 24, 2021 04:33

gaoxk merged commit 6e3fb13 into main Dec 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don’t return story translations if user already translating/reviewing for language #229

Don’t return story translations if user already translating/reviewing for language #229

gaoxk commented Dec 23, 2021

gaoxk Dec 23, 2021

Puepis Dec 24, 2021

gaoxk Dec 24, 2021

gaoxk Dec 23, 2021

github-actions bot commented Dec 23, 2021 •

edited

Loading

Puepis left a comment

Puepis Dec 24, 2021

Puepis Dec 24, 2021

gaoxk Dec 24, 2021

Don’t return story translations if user already translating/reviewing for language #229

Don’t return story translations if user already translating/reviewing for language #229

Conversation

gaoxk commented Dec 23, 2021

Notion ticket link

Implementation description

Steps to test

Homepage View

Translate

Review

Admin UserProfile View

Translate

Review

What should reviewers focus on?

Checklist

gaoxk Dec 23, 2021

Choose a reason for hiding this comment

Puepis Dec 24, 2021

Choose a reason for hiding this comment

gaoxk Dec 24, 2021

Choose a reason for hiding this comment

gaoxk Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot commented Dec 23, 2021 • edited Loading

Puepis left a comment

Choose a reason for hiding this comment

Puepis Dec 24, 2021

Choose a reason for hiding this comment

Puepis Dec 24, 2021

Choose a reason for hiding this comment

gaoxk Dec 24, 2021

Choose a reason for hiding this comment

github-actions bot commented Dec 23, 2021 •

edited

Loading