Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
StephenYu2018 opened this issue Apr 28, 2024 · 6 comments
Assignees
Labels
enhancement Label to indicate an issue is a feature/improvement

Comments

@StephenYu2018
Copy link
Contributor

StephenYu2018 commented Apr 28, 2024

Is your feature request related to a problem? Please describe.

When handling translation suggestion's target exploration images, the following GCS file system operations occur:

  1. When the translation is submitted, target exploration images are copied from GCS /exploration/<exp_id>/assets/images/ to /exploration_suggestions/<target_exp_id>/assets/images/.
  2. When reviewing the translation, to display the target exploration content, the target exploration's images are fetched from GCS exploration_suggestions/<target_exp_id>/assets/images/.

The target exploration images are only copied once and are fetched from their copied destination. What if the exploration editor adds new images to the target exploration? Those new images aren't going to show up in the target exploration content when reviewing associated translation suggestions, since they were never copied to the translation suggestions' assets.

Describe the solution (or solutions) you'd like

When handling translation suggestion's target exploration images, I would like the following steps to occur instead:

  1. When the translation is submitted, target exploration images remain at GCS /exploration/<exp_id>/assets/images/.
  2. When reviewing the translation, to display the target exploration content, the target exploration's images are fetched from GCS exploration/<exp_id>/assets/images/.

This way, when the exploration editor adds new images to the target exploration, those new images will show in the target exploration content when reviewing associated translation suggestions. This is assuming that updates to the target exploration HTML content by the exploration editor are reflected in the translation modal and the translation suggestion review modal.

Describe alternatives you've considered and rejected

No response

Additional context

This concern was brought up in PR #20188. However, I felt that the concern was separate from the issue linked to that PR, so I've opened this issue to address the concern mentioned in this review thread.

@seanlip
Copy link
Member

seanlip commented Apr 28, 2024

@StephenYu2018 Just a note -- for this issue, the fix would also include copying only the images used in the suggestion from the target exploration, not all the images there, correct?

Also, just a note (from the other PR) that we should also add a test where the source image is replaced by a different image in the suggestion (or where there are two images in the original text and only one is replaced and the other is kept) and verify that this is handled correctly in the backend (i.e. only the images used in the suggestion should be copied to the suggestion namespace).

@StephenYu2018
Copy link
Contributor Author

@seanlip Yes, that's correct. Only images created with the copy tool should be copied from GCS /exploration/ to /exploration_suggestions/.

@StephenYu2018
Copy link
Contributor Author

@seanlip It seems that there are already measures taken to address outdated translations when exploration content gets updated. Should I still go through with the feature that I mentioned above? Even if the translation suggestion review modal already shows both the outdated and updated exploration content?

Image

Regardless, I think I should change where the new content images are being fetched from so that they can load. It should be fetched from GCS /exploration/ not GCS /exploration_suggestions/.

@seanlip
Copy link
Member

seanlip commented May 13, 2024

@StephenYu2018 Yes, I think so. I think the current system is logically confusing (in the ccdebase) and we should fix that and clean up any "hacks" that don't conform to the logical model for where things should live / be fetched from.

Does that sound good to you, too?

@seanlip seanlip removed their assignment May 13, 2024
@StephenYu2018
Copy link
Contributor Author

@seanlip Sounds good. Although how will we update the UI? Should we only display the most-up-to-date exploration content at all times?

@seanlip
Copy link
Member

seanlip commented May 13, 2024

That is a good question ... I think yes, let's go ahead and do that. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Label to indicate an issue is a feature/improvement
Projects
Status: In Progress
Development

No branches or pull requests

2 participants