oppia / oppia

A free, online learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
5.63k stars 3.78k forks source link

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

Open StephenYu2018 opened 2 weeks ago

StephenYu2018 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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

StephenYu2018 commented 1 day ago

@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 commented 19 hours ago

@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?

StephenYu2018 commented 17 hours ago

@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 commented 17 hours ago

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