-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 part of #19864 : Added feature to generate dummy suggestion questions #20248
base: develop
Are you sure you want to change the base?
Conversation
Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, it adds a new cron job. |
Assigning @nikitaevg for the first pass review of this PR. Thanks! |
core/controllers/admin.py
Outdated
@@ -169,6 +170,8 @@ class AdminHandlerNormalizePayloadDict(TypedDict): | |||
collection_id: Optional[str] | |||
num_dummy_exps_to_generate: Optional[int] | |||
num_dummy_exps_to_publish: Optional[int] | |||
num_dummy_suggestion_ques_generate: Optional[int] |
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.
Please don't use short versions of words which are not widely acceptable. Here, ques is questions? No need to use ques.
I understand you might want to continue using num as in the fields above
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!
core/controllers/admin.py
Outdated
skill_summaries = skill_services.get_all_skill_summaries() | ||
skill_summary_dicts = [ | ||
summary.to_dict() for summary in skill_summaries] | ||
logging.error(skill_summary_dicts) |
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.
Please re-read your PRs before sending them
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!
Exception. User does not have enough rights to generate data. | ||
""" | ||
assert self.user_id is not None | ||
if constants.DEV_MODE: |
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's usually a very well serving pattern - handle corner cases first and common cases later.
Here
if not constants.DEV_MODE:
raise...
<main_code>
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.
@nikitaevg In the whole file it is written like this so I thought it is the appropriate method, so now do I need to change mine code as well as other part of file?
core/controllers/admin.py
Outdated
'<p>This is a hint.</p>')), | ||
] | ||
# Ruling out the possibility of None for mypy type checking, | ||
# because we above we are already updating the value |
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.
typo, please proofread PRs
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.
Also, this comment and one below - try to make comments concise. Comments should point out something extraordinary, and the longer the comment the more attention it attracts. These two comments are not very important, try to make it 1, at most 2 lines.
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!
state.interaction.default_outcome.labelled_as_correct = True | ||
state.interaction.default_outcome.dest = None | ||
suggestion_change: Dict[ | ||
str, Union[str, float, question_domain.QuestionDict] |
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 not use AcceptableChangeDictTypes instead of the Union?
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.
@nikitaevg Yeah, I had a doubt about this. If we change Union[str, float, question_domain.QuestionDict]
to change_domain.AcceptableChangeDictTypes
, how will we know the exact data type we are receiving? Using change_domain.AcceptableChangeDictTypes
doesn't make the code very generic. Currently, we know that we will only receive str, float, or question_domain.QuestionDict, but using AcceptableChangeDictTypes will not define that. Am I missing something about it?
( | ||
suggestion_services | ||
.update_question_contribution_stats_at_submission( | ||
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.
Why do you add parenthesis around this expression?
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 have added parenthesis because this was exceeding max line character limit, so to distribute it into 2 lines.
@@ -420,7 +420,7 @@ class ClassifierDict(TypedDict): | |||
# customization argument choices. | |||
INVALID_CONTENT_ID = 'invalid_content_id' | |||
# The default content text for the initial state of an exploration. | |||
DEFAULT_INIT_STATE_CONTENT_STR = '' | |||
DEFAULT_STATE_CONTENT_STR = '' |
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.
Please refrain from adding more changes to the PRs that are already not small. You complicate reviewers' life by adding more changes. Keep this, but in the future for any PR with >300 LOCs add only changes that are needed for this feature, you can refactor in the follow up 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.
Sure, from next time onwards I will remember that!
core/controllers/admin.py
Outdated
@@ -1226,6 +1270,115 @@ def _generate_dummy_classroom(self) -> None: | |||
else: | |||
raise Exception('Cannot generate dummy classroom in production.') | |||
|
|||
def _generate_dummy_suggestion_questions( |
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 tests, huh?
Unassigning @nikitaevg since the review is done. |
Hi @HardikGoyal2003, it looks like some changes were requested on this pull request by @nikitaevg. 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.
Several concerns.
core/templates/pages/admin-page/activities-tab/admin-dev-mode-activities-tab.component.spec.ts
Outdated
Show resolved
Hide resolved
core/controllers/admin.py
Outdated
or not user_services.can_submit_question_suggestions( | ||
self.user_id)): | ||
raise Exception( | ||
'User does not have enough rights to generate data.') |
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.
Rename error message to User "${self.username}" must be a question submitter or question admin in order to generate 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.
Done!
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.
Wait, my bad. Can you replace the JS string interpolation syntax in my suggestion with Python's string interpolation (either with %
operator or f-strings)?
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!
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 still don't think that the syntax is correct. I think it should be:
raise Exception((
'User "%s" must be a question submitter or question admin'
' in order to generate question suggestions.'
) % self.username)
Also, do you have a backend test written that covers this part of the code? If you do have one, then running that test would also catch any syntax errors with this part of the code.
Hi @HardikGoyal2003, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Hi @HardikGoyal2003. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
Screencast.from.28-04-24.08.16.18.PM.IST.webm
PR Pointers