Fix #19110: Copy translation's target exploration images regardless if submitter uploaded new images or not #20188

StephenYu2018 · 2024-04-20T02:57:49Z

Overview

This PR fixes #19110.

This PR does the following:

Instructs SuggestionHandler to copy of translation's target exploration images within GCS file system from /exploration/... to /exploration_suggestions/...
- This makes images created using the copy tool able to load when reviewing the translation suggestion
Refactors relevant code flow starting from SuggestionHandler.post()
cloud_storage_services.py and dev_mode_storage_services.py throw a ValueError if source asset does not exist at given filepath
Introduces backend tests:
- cloud_storage_services_test: Throws value error when source asset does not exist at filepath
- suggestion_test:
  - test_saves_new_images_in_translation_that_were_copied_from_target_exploration
  - test_raises_exception_if_target_exploration_image_is_not_in_target_exploration_assets

The original bug occurred because images in the translation's target exploration were copied in GCS File System from /exploration/... to /exploration_suggestions/... only when the translation submitter uploaded new images to the translation text. If a translation reviewer were to review that submitted translation, the images would not load since they must be fetched from /exploration_suggestions/....

The PR that introduced this bug is #16532, in particular this commit. Prior to these changes, suggestion images were uploaded as long as it wasn't a question suggestion (in other words, a translation suggestion). Those changes introduced a check to only upload suggestion images if the list of new images introduced by the translation submitter was not empty. It's also possible that it was trying to check that the list of new images wasn't None.

Essential Checklist

Please follow the instructions for making a code change.

I have linked the issue that this PR fixes in the "Development" section of the sidebar.
I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
I have written tests for my code.
The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

Before:

After:

Proof of changes on desktop with slow/throttled network

issue-19110-slow-proof.mp4

Proof of changes on mobile phone

issue-19110-mobile-proof.mp4

Proof of changes in Arabic language

issue-19110-arabic-proof.mp4

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

…dless if there are new images introduced in the translation suggestion or not.

…copy from a source asset that does not exist.

… exist.

oppiabot · 2024-04-20T02:57:51Z

Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, it modifies files in jobs or platform folders.
Also @StephenYu2018, please make sure to fill in the server jobs form for the new job or feature to be tested on the backup server. This PR can be merged only after the test run is successful. Please refer to this guide for details.
Thanks!

oppiabot · 2024-04-20T02:58:01Z

Assigning @chris7716 for the first pass review of this PR. Thanks!

StephenYu2018 · 2024-04-20T03:02:32Z

@chris7716 @kevintab95 If the only datastore change is to raise an error when copying files in the case of the given source asset not existing, do I still have to do server testing for this behavior? If so, then I think I'll just revert this behavior back to what it originally was, since I'm aiming to get this PR merged by April 30th latest.

seanlip · 2024-04-20T15:19:46Z

core/controllers/suggestion.py

+            filenames: list(str). The image filenames.
+        """
+        suggestion_image_context = suggestion.image_context
+        # TODO(#10513): Find a way to save the images before the suggestion is


Just to check, is this still relevant here? From reading this it's a bit unclear when the suggestion is created exactly, and what we actually want to do here.

@seanlip Issue #10513 is still open as of now. Although I personally don't see why we should save the images before the suggestion is saved, with the exception that images for question suggestions are saved before saving the question suggestion. I think that images should be saved as the suggestion is saved, since those images are a part of the suggestion and it doesn't make sense for images uploaded by the translation submitter to exist without the suggestion existing first. What do you think?

Yes, I think that sounds reasonable to me too. I think we can close that issue since it doesn't provide justification and we can't understand why it makes conceptual sense to do it ... I'll leave a note there.

(Btw the comments in #10475 have some interesting context on the existing implementation, e.g. https://docs.google.com/document/d/1kGvPjYnW9ArYKtzxYOmFbp7MA-A45B3znRWFIlaEGVo/edit#heading=h.xyfn6zlrzj3e and #10475 (comment)

@seanlip I removed the TODO comment.

seanlip · 2024-04-20T15:20:49Z

core/controllers/suggestion.py

+            )
+        except ValueError as error:
+            raise Exception(
+                'There are images in the target exploration that are '


This is somewhat unclear and I wouldn't know what to do if I saw this error. Could you please explain it more simply?

An image in the submitted translation's original content named "{filename}" cannot be found. Please save it to the backend file system at /exploration/{exp_id}/assets/image/ before submitting this translation again.

For example, if an image with filename img.png was part of submitted translation's target exploration with id s5Fr3Zd, then the error message will read: An image in the submitted translation's original content named "img.png" cannot be found. Please save it to the backend file system at /exploration/s5Fr3Zd/assets/image/ before submitting this translation again.

Also, I think I should move the copy operation of old images before the save operation of new images. That way if the images aren't copied successfully, then we don't save the new images either.

@seanlip What do you think?

Yup, that's much clearer, thanks!

I don't have a strong opinion on which order we should do the copy/save in -- I'm not sure why we'd want to test copying first in particular. One suggestion to keep things clean might be to fetch all the necessary images first (from both the original content and the new suggestion), and then save all of them to the correct paths in one for loop.

@seanlip I restructured the error message.

seanlip · 2024-04-20T15:23:17Z

core/controllers/suggestion.py

                    files, suggestion, new_image_filenames
                )

+            self._copy_images_from_target_exploration_to_translation(suggestion)


I'm a bit confused ... why do we upload and then copy? I.e. why isn't adding the images that were part of the translation suggestion into the suggestion's directory sufficient?

I would have thought that we would:

(a) add the images that were sent as image data into the suggestion's directory
(b) copy the images that were part of the translation suggestion's text, but that were not sent as image data, into the suggestion's directory

@seanlip For context, the copy tool creates an image in the HTML string, whose filepath points to the original image's filepath. The original image is an image in a card's content belonging to the target exploration. The intent of this algorithm isn't to upload images created using the copy tool. Instead, we are trying to copy the original images for the translation suggestion to use as assets. Since the images created using the copy tool have the same filename as the original image, this will also allow the images created using the copy tool to load when reviewing the translation. In short, we save images uploaded directly from the translation submitter, and then we copy the original images within the target exploration's card's content for the suggestion to use later on.

I think the highlighted method name is not clear about this. I think the name _copy_images_in_target_exploration_content_to_translation(suggestion) would be better since it better explains what images I'm referring to.

Ah yup, as long as we just use the images in the translation suggestion's card, then that seems fine. I just want to ensure we aren't copying "extra" images that aren't part of the card. Thanks!

@seanlip I renamed the method responsible for copying the images in the target exploration content.

kevintab95

LGTM for 4 codeowner files. Thanks @StephenYu2018!

oppiabot · 2024-04-22T09:36:49Z

Assigning @lkbhitesh07 for code owner reviews. Thanks!

…ted.

seanlip

Thanks @StephenYu2018, replied to both comments (including here).

seanlip · 2024-04-27T17:30:31Z

core/controllers/suggestion_test.py

+            response_dict['error'])
+        self.logout()
+
+    def test_saves_new_images_in_translation_that_were_copied_from_target_exploration( # pylint: disable=line-too-long


@StephenYu2018 Sorry, I can't find that test ... which line of code are you referring to?

seanlip

Thanks @StephenYu2018! LGTM, we can defer future work to #20232 as discussed.

chris7716

LGTM for codeowner files.

oppiabot · 2024-04-29T17:51:17Z

Unassigning @chris7716 since they have already approved the PR.

oppiabot · 2024-04-29T17:51:20Z

Hi @StephenYu2018, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

StephenYu2018 · 2024-04-30T22:09:16Z

@seanlip @chris7716 @kevintab95 Can you merge this PR now? The CI checks have passed.

StephenYu2018 added 4 commits April 19, 2024 10:04

Move backend code out of if statement so that images are copied regar…

472fd52

…dless if there are new images introduced in the translation suggestion or not.

Add check for GCS in Production to raise an exception when trying to …

db80176

…copy from a source asset that does not exist.

Re-raise exception from GCS COPY operation when source asset does not…

7c19125

… exist.

Merge branch 'develop' into issue-19110

925c6ce

StephenYu2018 requested review from a team as code owners April 20, 2024 02:57

StephenYu2018 requested review from chris7716 and lkbhitesh07 April 20, 2024 02:57

oppiabot bot assigned DubeySandeep, kevintab95 and vojtechjelinek Apr 20, 2024

oppiabot bot added the PR: Affects datastore layer Labels to indicate a PR that changes the datastore layer. label Apr 20, 2024

oppiabot bot assigned chris7716 Apr 20, 2024

StephenYu2018 added 2 commits April 19, 2024 23:02

Merge branch 'develop' into issue-19110

6f1b74b

Merge branch 'develop' into issue-19110

f418831

seanlip requested changes Apr 20, 2024

View reviewed changes

seanlip assigned StephenYu2018 Apr 20, 2024

seanlip mentioned this pull request Apr 20, 2024

Save the images for suggestions before the suggestions are saved in the datastore. #10513

Closed

kevintab95 approved these changes Apr 22, 2024

View reviewed changes

kevintab95 removed their assignment Apr 22, 2024

oppiabot bot assigned lkbhitesh07 Apr 22, 2024

StephenYu2018 assigned seanlip Apr 22, 2024

Merge branch 'develop' into issue-19110

685f38a

seanlip removed their assignment Apr 22, 2024

StephenYu2018 added 2 commits April 22, 2024 18:21

Merge branch 'develop' into issue-19110

3f68971

Remove TODO comment for saving images before the suggestion is submit…

aea2df2

…ted.

lkbhitesh07 removed their assignment Apr 27, 2024

Merge branch 'develop' into issue-19110

1709bc2

seanlip reviewed Apr 27, 2024

View reviewed changes

seanlip removed their assignment Apr 27, 2024

StephenYu2018 assigned seanlip Apr 28, 2024

StephenYu2018 mentioned this pull request Apr 28, 2024

[Blocked on #19110] [Feature Request]: Translation suggestion's target exploration images should belong to the exploration instead of the suggestion #20232

Open

StephenYu2018 assigned Ash-2k3 and unassigned Ash-2k3 Apr 28, 2024

StephenYu2018 requested a review from seanlip April 28, 2024 13:10

seanlip approved these changes Apr 28, 2024

View reviewed changes

seanlip removed their assignment Apr 28, 2024

Merge branch 'develop' into issue-19110

9dad8c2

chris7716 approved these changes Apr 29, 2024

View reviewed changes

oppiabot bot unassigned chris7716 Apr 29, 2024

oppiabot bot added the PR: LGTM label Apr 29, 2024

StephenYu2018 added 2 commits April 29, 2024 14:01

Merge branch 'develop' into issue-19110

1401fd5

Merge branch 'develop' into issue-19110

269d788

StephenYu2018 mentioned this pull request Apr 30, 2024

[Flake]: Topics tab took too long to be clickable when running curriculum admin acceptance test #20238

Closed

Merge branch 'develop' into issue-19110

492f813

StephenYu2018 assigned seanlip, kevintab95 and chris7716 Apr 30, 2024

seanlip added this pull request to the merge queue May 1, 2024

Merged via the queue into oppia:develop with commit bcb13ff May 1, 2024
81 checks passed

StephenYu2018 deleted the issue-19110 branch May 1, 2024 18:05

U8NWXD mentioned this pull request Dec 4, 2024

Fix #19110: Implement job to copy mis-placed translation images #21378

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #19110: Copy translation's target exploration images regardless if submitter uploaded new images or not #20188

Fix #19110: Copy translation's target exploration images regardless if submitter uploaded new images or not #20188

StephenYu2018 commented Apr 20, 2024 •

edited

Loading

oppiabot bot commented Apr 20, 2024

oppiabot bot commented Apr 20, 2024

StephenYu2018 commented Apr 20, 2024 •

edited

Loading

seanlip Apr 20, 2024

StephenYu2018 Apr 20, 2024

seanlip Apr 20, 2024

seanlip Apr 20, 2024

StephenYu2018 Apr 23, 2024

seanlip Apr 20, 2024

StephenYu2018 Apr 22, 2024

seanlip Apr 22, 2024

StephenYu2018 Apr 23, 2024

seanlip Apr 20, 2024

StephenYu2018 Apr 20, 2024 •

edited

Loading

seanlip Apr 22, 2024 •

edited

Loading

StephenYu2018 Apr 23, 2024 •

edited

Loading

kevintab95 left a comment

oppiabot bot commented Apr 22, 2024

seanlip left a comment

seanlip Apr 27, 2024

seanlip left a comment

chris7716 left a comment

oppiabot bot commented Apr 29, 2024

oppiabot bot commented Apr 29, 2024

StephenYu2018 commented Apr 30, 2024

Fix #19110: Copy translation's target exploration images regardless if submitter uploaded new images or not #20188

Fix #19110: Copy translation's target exploration images regardless if submitter uploaded new images or not #20188

Conversation

StephenYu2018 commented Apr 20, 2024 • edited Loading

Overview

Essential Checklist

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

oppiabot bot commented Apr 20, 2024

oppiabot bot commented Apr 20, 2024

StephenYu2018 commented Apr 20, 2024 • 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

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

StephenYu2018 Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

seanlip Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

StephenYu2018 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

kevintab95 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 22, 2024

seanlip left a 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

chris7716 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 29, 2024

oppiabot bot commented Apr 29, 2024

StephenYu2018 commented Apr 30, 2024

StephenYu2018 commented Apr 20, 2024 •

edited

Loading

StephenYu2018 commented Apr 20, 2024 •

edited

Loading

StephenYu2018 Apr 20, 2024 •

edited

Loading

seanlip Apr 22, 2024 •

edited

Loading

StephenYu2018 Apr 23, 2024 •

edited

Loading