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

Fix #14235: Enable to submit question suggestions with image region interations #14289

Merged
merged 38 commits into from
Nov 26, 2021

Conversation

chris7716
Copy link
Contributor

@chris7716 chris7716 commented Nov 19, 2021

Overview

  1. This PR fixes Unable to submit image region question #14235.
  2. This PR does the following:
  • sets a custom entity context when suggesting question
  • sets image save destination to IMAGE_SAVE_DESTINATION_SERVER when submitting questions

Essential Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

Proof that changes are correct

qe.mov

PR Pointers

  • Make sure to follow the instructions for making a code change.
  • Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
  • Never force push. If you do, your PR will be closed.
  • Oppiabot can assign anyone for review/help if you leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL"
  • Some of the 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.

Sorry, something went wrong.

@chris7716 chris7716 requested a review from a team November 19, 2021 22:05
@oppiabot
Copy link

oppiabot bot commented Nov 19, 2021

Hi @chris7716, can you complete the following:

  1. The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
    Thanks!

@oppiabot
Copy link

oppiabot bot commented Nov 19, 2021

Hi @vojtechjelinek, @seanlip -- could one of you please add the appropriate changelog label to this pull request? Thanks!

@chris7716
Copy link
Contributor Author

@seanlip PTAL thanks!

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chris7716, thanks for the PR! To be honest, though, I'm not sure I understand several parts of what is going on.

I left some comments -- could you please explain? Thanks!

this.contextService.getEntityType() !== (
AppConstants.ENTITY_TYPE.EXPLORATION)
) {
const entityType: string = this.contextService.getEntityType() || '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about this. '' does not seem like a reasonable value for entityType...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'' was added in order to prevent a lint error since the return type of getEntityType() is string | undefined but assetsBackendApiService.getImageUrlForPreview() only expects a string as getEntityType().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's still not OK. In what cases can undefined happen? We should try to prevent those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the current path name doesn't include any of create explore embed topic_editor story_editor skill_editor blog-dashboard or no custom context entity is set, getEntityType() will return undefined. SInce a custome context entity is being set in this case, it should not return an undefine.

Copy link
Member

@seanlip seanlip Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I think you need to do something like -- just call getEntityType() (no backup of ''), and if it's undefined, throw an error. (Should add comments to explain what's going on, too.)

But conceptually allowing '' doesn't seem OK.

elif entity_type == feconf.ENTITY_TYPE_SKILL:
elif (
entity_type == feconf.ENTITY_TYPE_SKILL or
entity_type == feconf.IMAGE_CONTEXT_QUESTION_SUGGESTIONS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems conceptually wrong to me. If I'm making suggestions to a question, shouldn't this relate to updating the question rather than updating the skill?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this one. Yes, it should be related to updating the question.

@@ -1641,59 +1641,6 @@ def test_suggestion_creation_when_images_are_not_provided(self):
response_dict['error'])
self.logout()

def test_suggestion_creation_when_images_are_not_valid(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial test was added for question suggestion. Since images are sent before the question is submitted this is not happening actually and I added this test with as translation suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must images be stored to the backend before the question is submitted? What happens if they're stored and then the question edit is cancelled -- will those images still remain in the backend?

If so can things be modified such that that is not the case?

Copy link
Contributor Author

@chris7716 chris7716 Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Must images be stored to the backend before the question is submitted? Yes in this case.

  2. What happens if they're stored and then the question edit is cancelled -- will those images still remain in the backend? As it stands, images will still remain in the backend if the question edit is cancelled.

  3. If so can things be modified such that that is not the case? As things stand in fs_services.py I can not find a function to remove saved images. Perhaps we may need to implement a function to do this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not save the images in the backend until the "submit question" button is pressed? (And, until then, can they just be saved in local storage in the frontend?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's the way to go but I am blocked while implementing as I said here #14289 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris7716 Do you still need anything from me for this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. What you said in the offline discussion is enough and I think we are going to implement this in the Stage 2. Thanks.

@@ -54,7 +56,11 @@ angular.module('oppia').controller('QuestionSuggestionEditorModalController', [
$scope.misconceptionsBySkill = {};
$scope.misconceptionsBySkill[$scope.skill.getId()] = (
$scope.skill.getMisconceptions());
ContextService.setImageSaveDestinationToLocalStorage();
ContextService.resetImageSaveDestination();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain (conceptually) what the old code was doing, and why this change had to be made?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My findings on this bug is as following.

  1. The initial error was thrown from this line of image-with-regions-editor.component.ts while calling the this.contextService.getExplorationId(). Since no exploration id is set, the error was thrown. getPreviewUrl() was called by the <image> in the image-with-regions-editor.component.html to set href value.
  2. When image saving destination is set to ContextService.setImageSaveDestinationToLocalStorage() images are temporarily stored in the web browser and all images are sent to the server when the question is submitted. But in this case, inorder to get the reference link to the href of <image>, we need to send the image to the server before the question is submitted. Hence this change was necessary in order to save images in the server before the question is submitted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, thanks for explaining. You definitely need comments in the code here, at the very least.

Some thoughts:

  1. I feel like the idea of "resetting" doesn't quite make sense here, conceptually. Shouldn't we just set it?
  2. Do we need to look into generalize the this.contextService.getExplorationId() call, if the image-with-regions-editor component isn't always going to appear in an exploration context? That seems like the better way to go IMO?
  3. Can you explain why we need to "get the reference link to the href of <image>"? Also I think "saving images in the server before the question is submitted" is probably fine but we must make sure that this only happens after we've committed to saving the question (i.e. the user can't cancel between the images being saved and the actual question being submitted).

Also, where are images being saved for question suggestions? Is it in some temporary place that's suggestion-related and then they are copied over to the main place where question images are located, once the question is accepted? If so, then are the temporary images deleted? Just trying to understand the full workflow, so that I can advise better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of keeping image saving destination to local storage as it was and that would reduce the complexity of the solution. If we can get the imageDataURI which is defined here and return it from getPreviewUrl () from here. But I am blocked how to get the imageDataURI from getPreviewUrl (). Could you please help me on this?

Copy link
Member

@seanlip seanlip Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need more info in order to help. Could you please answer my previous questions thoroughly in more detail?

If we have a better common understanding of how the system is working, then it will be easier to find the right solution.

Copy link
Contributor Author

@chris7716 chris7716 Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the earlier comment without fully explaining for what was asked initially.

  1. I think we do not need to reset or set the image saving destination, since there is a default destination defined i.e. AppConstants.IMAGE_SAVE_DESTINATION_SERVER which is exactly what is done in ContextService.resetImageSaveDestination() -- probably can remove this line.
  2. In order to eliminate this, I set a customeContextEntity and the entity id set over there is taken in getPreviewUrl() instead of this.contextService.getExplorationId() -- link.
  3. My initial assumption was wrong here -- we still can use imageDataURI which is defined here as the source for the <image> so that we do not need to send image to the server before submitting the question. But as I said in my previous comment, I am not able to get the imageDataURI of the selected image to the image-with-regions-editor component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we still resetting here?

@seanlip seanlip removed their assignment Nov 20, 2021
Comment on lines 317 to 321
if (
this.contextService.getEntityType() !== (
AppConstants.ENTITY_TYPE.EXPLORATION)
) {
const entityType: string = this.contextService.getEntityType() as string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do this with a single function call to this.contextService.getEntityType(). Use a separate variable before the if condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@iamprayush iamprayush removed their assignment Nov 26, 2021
@oppiabot
Copy link

oppiabot bot commented Nov 26, 2021

Hi @chris7716, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks!

Copy link
Contributor Author

@chris7716 chris7716 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanlip @gp201 @iamprayush PTAL. Thanks!

suggestion_models.STATUS_ACCEPTED)
# Checks whether image of the Image Region interaction is accessible
# from the question player.
destination_fs = fs_domain.AbstractFileSystem(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A pre-check can not be included here since we can not get the directory(/question/question_id/). This directory is only created when the suggestion is accepted.


# Image for interactions with Image Regions in not included as an html
# string. This image is as the imagePath in customization args.
if question.question_state_data.interaction.id == 'ImageClickInput':
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They have ck-editor that will include the reference of images so that those image names can be recieved from the html string.

Copy link
Contributor

@gp201 gp201 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for codeowner files

@gp201 gp201 removed their assignment Nov 26, 2021
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my end. Thanks @chris7716 !

@seanlip seanlip removed their assignment Nov 26, 2021
Copy link
Contributor

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Left a few nits, feel free to address them in subsequent PR.

@@ -314,6 +314,15 @@ export class ImageWithRegionsEditorComponent implements OnInit {
}

getPreviewUrl(imageUrl: string): string {
const entityType: string = this.contextService.getEntityType() as string;
if (
entityType !== (AppConstants.ENTITY_TYPE.EXPLORATION)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
entityType !== (AppConstants.ENTITY_TYPE.EXPLORATION)
entityType !== AppConstants.ENTITY_TYPE.EXPLORATION

@@ -314,6 +314,15 @@ export class ImageWithRegionsEditorComponent implements OnInit {
}

getPreviewUrl(imageUrl: string): string {
const entityType: string = this.contextService.getEntityType() as string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work the same

Suggested change
const entityType: string = this.contextService.getEntityType() as string;
const entityType = this.contextService.getEntityType() as string;

Comment on lines +255 to +279
# Image context: Question Suggestions.
self.login(self.CURRICULUM_ADMIN_EMAIL)
csrf_token = self.get_new_csrf_token()

with python_utils.open_file(
os.path.join(feconf.TESTS_DATA_DIR, 'img.png'), 'rb',
encoding=None
) as f:
raw_image = f.read()
response_dict = self.post_json(
'%s/question_suggestions/%s' % (
self.IMAGE_UPLOAD_URL_PREFIX,
skill_id
),
{'filename': 'test.png'},
csrf_token=csrf_token,
upload_files=(('image', 'unused_filename', raw_image),)
)
filename = response_dict['filename']

self.logout()

response = self.get_custom_response(
self._get_image_url('skill', skill_id, filename), 'image/png')
self.assertEqual(response.body, raw_image)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to put this into separate test function.

@oppiabot
Copy link

oppiabot bot commented Nov 26, 2021

Unassigning @vojtechjelinek since they have already approved the PR.

Copy link
Contributor

@iamprayush iamprayush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@iamprayush iamprayush merged commit 0958ade into oppia:develop Nov 26, 2021
seanlip pushed a commit that referenced this pull request Nov 27, 2021
…nterations (#14289)

* First pass

* Fix lint

* Fix lint

* Fix ts checks

* Fix lint

* Fix f-e test

* Fix f-e test

* Fix backend tests

* Fix backend test

* Fix lints

* Fix backend tests

* Fix lints

* Fix backend tests

* Fix lints

* Fix backend coverage

* Fix backend tests

* Copy images to required destination after acceptance

* Fix lint

* Fix ts checks

* Remove image destination reset

* Address reviews

* Fix backend errors

* Fix backend errors

* Fix backend errors

* Fix backend errors

* Fix backend errors

* Fix lint

* Fix backend tests

* Fix lint

* Fix nits

* Add more backend tests

* Fix lints

* Address review requests

* Fix nits

* Fix nits

* Fix b-e tests
srijanreddy98 pushed a commit to srijanreddy98/oppia that referenced this pull request Nov 27, 2021
…ppia#14058)

Fix oppia#14235: Enable to submit question suggestions with image region interations (oppia#14289)

* First pass

* Fix lint

* Fix lint

* Fix ts checks

* Fix lint

* Fix f-e test

* Fix f-e test

* Fix backend tests

* Fix backend test

* Fix lints

* Fix backend tests

* Fix lints

* Fix backend tests

* Fix lints

* Fix backend coverage

* Fix backend tests

* Copy images to required destination after acceptance

* Fix lint

* Fix ts checks

* Remove image destination reset

* Address reviews

* Fix backend errors

* Fix backend errors

* Fix backend errors

* Fix backend errors

* Fix backend errors

* Fix lint

* Fix backend tests

* Fix lint

* Fix nits

* Add more backend tests

* Fix lints

* Address review requests

* Fix nits

* Fix nits

* Fix b-e tests

Fixes part of oppia#9749: Migrate few instances of angular-html-bind (oppia#14263)

* Revert "Revert "Fix part of oppia#9749: Create a component that shows rte-output-display and use it instead of angular-html-bind. (oppia#13229)" (oppia#13361)"

This reverts commit 792de45.

* Fix issues

* Fix spelling mistakes in comment

* Merge with upstream/develop

* Address review comments:
  1. Move oppia-rte-output.spec from the list of ignored files for innerHTML check in eslintrc and move to inline disables.
  2. Change the CODEOWNER listing from oppia-rte-parser.service.ts to oppia-rte-parser.service*.ts to include the spec file.
  3. Add refernce to the note at the top of file for all value.replace ops.
  4. Miscellaneous changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to submit image region question
6 participants