Fix part of #19864 : Added feature to generate dummy suggestion questions #20248

HardikGoyal2003 · 2024-05-01T21:49:44Z

Overview

This PR fixes or fixes part of [Feature Request]: Add dummy review translation and review question option #19864
This PR does the following:
- Adds a feature to generate dummy suggestion question
- Adds 'N' number of suggestion questions to specified skill.

Essential Checklist

Please follow the instructions for making a code change.

I have linked the issue that this PR fixes in the "Development" section of the sidebar.
I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
I have written tests for my code.
The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

Screencast.from.28-04-24.08.16.18.PM.IST.webm

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some 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.
See the Code Owner's wiki page for what code owners will expect.

oppiabot · 2024-05-01T21:49:46Z

Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, it adds a new cron job.
Also @HardikGoyal2003 It seems you have added or edited a CRON job, if so please request a testing of this CRON job with this server jobs form Please refer to this guide for details.
Thanks!

oppiabot · 2024-05-01T21:49:56Z

Assigning @nikitaevg for the first pass review of this PR. Thanks!

nikitaevg · 2024-05-02T17:55:11Z

core/controllers/admin.py

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


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

nikitaevg · 2024-05-02T17:56:47Z

core/controllers/admin.py

+        skill_summaries = skill_services.get_all_skill_summaries()
+        skill_summary_dicts = [
+            summary.to_dict() for summary in skill_summaries]
+        logging.error(skill_summary_dicts)


Please re-read your PRs before sending them

nikitaevg · 2024-05-02T17:59:42Z

core/controllers/admin.py

+            Exception. User does not have enough rights to generate data.
+        """
+        assert self.user_id is not None
+        if constants.DEV_MODE:


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>

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

nikitaevg · 2024-05-02T18:00:43Z

core/controllers/admin.py

+                            '<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


typo, please proofread PRs

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.

nikitaevg · 2024-05-02T18:13:20Z

core/controllers/admin.py

+                state.interaction.default_outcome.labelled_as_correct = True
+                state.interaction.default_outcome.dest = None
+                suggestion_change: Dict[
+                    str, Union[str, float, question_domain.QuestionDict]


Why not use AcceptableChangeDictTypes instead of the Union?

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

nikitaevg · 2024-05-02T18:15:18Z

core/controllers/admin.py

+                (
+                    suggestion_services
+                    .update_question_contribution_stats_at_submission(
+                        suggestion))


Why do you add parenthesis around this expression?

I have added parenthesis because this was exceeding max line character limit, so to distribute it into 2 lines.

nikitaevg · 2024-05-02T18:17:30Z

core/feconf.py

@@ -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 = ''


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

Sure, from next time onwards I will remember that!

core/templates/domain/admin/admin-backend-api.service.spec.ts

nikitaevg · 2024-05-02T18:26:17Z

core/controllers/admin.py

@@ -1226,6 +1270,115 @@ def _generate_dummy_classroom(self) -> None:
        else:
            raise Exception('Cannot generate dummy classroom in production.')

+    def _generate_dummy_suggestion_questions(


No tests, huh?

oppiabot · 2024-05-02T18:29:24Z

Unassigning @nikitaevg since the review is done.

oppiabot · 2024-05-02T18:29:27Z

Hi @HardikGoyal2003, it looks like some changes were requested on this pull request by @nikitaevg. PTAL. Thanks!

StephenYu2018

Several concerns.

core/controllers/admin.py

core/templates/pages/admin-page/activities-tab/admin-dev-mode-activities-tab.component.spec.ts

core/templates/domain/admin/admin-backend-api.service.spec.ts

core/controllers/admin.py

StephenYu2018 · 2024-05-03T13:46:52Z

core/controllers/admin.py

+                or not user_services.can_submit_question_suggestions(
+                    self.user_id)):
+                raise Exception(
+                    'User does not have enough rights to generate data.')


Rename error message to User "${self.username}" must be a question submitter or question admin in order to generate question suggestions.

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

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.

core/templates/domain/admin/admin-backend-api.service.spec.ts

oppiabot · 2024-05-11T10:33:03Z

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.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

Added Dummy Suggestion feature

70371aa

HardikGoyal2003 requested review from a team as code owners May 1, 2024 21:49

HardikGoyal2003 requested a review from nikitaevg May 1, 2024 21:49

HardikGoyal2003 requested a review from StephenYu2018 May 1, 2024 21:49

oppiabot bot assigned DubeySandeep, kevintab95 and vojtechjelinek May 1, 2024

oppiabot bot added the PR: Affects datastore layer Labels to indicate a PR that changes the datastore layer. label May 1, 2024

oppiabot bot assigned nikitaevg May 1, 2024

nikitaevg requested changes May 2, 2024

View reviewed changes

oppiabot bot unassigned nikitaevg May 2, 2024

oppiabot bot assigned HardikGoyal2003 May 2, 2024

StephenYu2018 requested changes May 3, 2024

View reviewed changes

DubeySandeep removed their assignment May 6, 2024

oppiabot bot added the stale label May 11, 2024

oppiabot bot closed this May 15, 2024

AFZL210 reopened this May 18, 2024

oppiabot bot removed the stale label May 18, 2024

HardikGoyal2003 added 2 commits May 19, 2024 01:30

Merge branch 'develop' into dummy-ques

11f28fb

lint fix

dd66179

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #19864 : Added feature to generate dummy suggestion questions #20248

Fix part of #19864 : Added feature to generate dummy suggestion questions #20248

HardikGoyal2003 commented May 1, 2024

oppiabot bot commented May 1, 2024

oppiabot bot commented May 1, 2024

nikitaevg May 2, 2024

HardikGoyal2003 May 18, 2024

nikitaevg May 2, 2024

HardikGoyal2003 May 18, 2024

nikitaevg May 2, 2024

HardikGoyal2003 May 19, 2024

nikitaevg May 2, 2024

nikitaevg May 2, 2024

HardikGoyal2003 May 19, 2024

nikitaevg May 2, 2024

HardikGoyal2003 May 19, 2024

nikitaevg May 2, 2024

HardikGoyal2003 May 19, 2024

nikitaevg May 2, 2024

HardikGoyal2003 May 19, 2024

nikitaevg May 2, 2024

oppiabot bot commented May 2, 2024

oppiabot bot commented May 2, 2024

StephenYu2018 left a comment

StephenYu2018 May 3, 2024

HardikGoyal2003 May 19, 2024

StephenYu2018 May 20, 2024

HardikGoyal2003 May 24, 2024

StephenYu2018 May 25, 2024

oppiabot bot commented May 11, 2024

Fix part of #19864 : Added feature to generate dummy suggestion questions #20248

Are you sure you want to change the base?

Fix part of #19864 : Added feature to generate dummy suggestion questions #20248

Conversation

HardikGoyal2003 commented May 1, 2024

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented May 1, 2024

oppiabot bot commented May 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented May 2, 2024

oppiabot bot commented May 2, 2024

StephenYu2018 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented May 11, 2024