-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
Hi @chris7716, can you complete the following:
|
Hi @vojtechjelinek, @seanlip -- could one of you please add the appropriate changelog label to this pull request? Thanks! |
@seanlip PTAL thanks! |
There was a problem hiding this 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() || ''; |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
core/controllers/acl_decorators.py
Outdated
elif entity_type == feconf.ENTITY_TYPE_SKILL: | ||
elif ( | ||
entity_type == feconf.ENTITY_TYPE_SKILL or | ||
entity_type == feconf.IMAGE_CONTEXT_QUESTION_SUGGESTIONS |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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? Yes in this case.
-
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.
-
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
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
- The initial error was thrown from this line of
image-with-regions-editor.component.ts
while calling thethis.contextService.getExplorationId()
. Since no exploration id is set, the error was thrown.getPreviewUrl()
was called by the<image>
in theimage-with-regions-editor.component.html
to set href value. - 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.
There was a problem hiding this comment.
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:
- I feel like the idea of "resetting" doesn't quite make sense here, conceptually. Shouldn't we just set it?
- 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? - 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- 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 inContextService.resetImageSaveDestination()
-- probably can remove this line. - In order to eliminate this, I set a customeContextEntity and the entity id set over there is taken in
getPreviewUrl()
instead ofthis.contextService.getExplorationId()
-- link. - 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 theimageDataURI
of the selected image to the image-with-regions-editor component.
There was a problem hiding this comment.
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?
if ( | ||
this.contextService.getEntityType() !== ( | ||
AppConstants.ENTITY_TYPE.EXPLORATION) | ||
) { | ||
const entityType: string = this.contextService.getEntityType() as string; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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! |
There was a problem hiding this 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( |
There was a problem hiding this comment.
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': |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
There was a problem hiding this 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 !
There was a problem hiding this 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
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
const entityType: string = this.contextService.getEntityType() as string; | |
const entityType = this.contextService.getEntityType() as string; |
# 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) |
There was a problem hiding this comment.
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.
Unassigning @vojtechjelinek since they have already approved the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…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
…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.
Overview
IMAGE_SAVE_DESTINATION_SERVER
when submitting questionsEssential Checklist
Proof that changes are correct
qe.mov
PR Pointers