Fix #16069: Add a "Newest First" tag for reviewable questions #16534

qinghaoyang · 2022-11-13T03:51:05Z

Overview

This PR fixes or fixes part of Sorting/Tagging questions by date #16069.
This PR does the following: Added a filter to sort the reviewable questions by selected keys. Currently, the keys include 'Date', which sorts the questions with the newest first.

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

Screen.Recording.2022-12-29.at.8.14.44.PM.mov

The change does not create issues when clicking other tabs.

Screen.Recording.2023-01-08.at.5.08.52.PM.mov

PR Pointers

Make sure to follow the instructions for making a code change.
If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
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.
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.

oppiabot · 2022-11-13T03:51:06Z

Hi @qinghaoyang, can you complete the following:

The body of this PR is missing the required description, please update the body with a description of what this PR does.
The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
Thanks!

oppiabot · 2022-11-13T03:51:17Z

Hi, @qinghaoyang, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR pointers. Assigning @qinghaoyang to add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

oppiabot · 2022-11-15T22:08:04Z

Hi @qinghaoyang, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

oppiabot · 2022-11-19T00:35:25Z

Hi @qinghaoyang. 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!

oppiabot · 2022-11-26T00:54:50Z

Hi @qinghaoyang, 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!

oppiabot · 2022-11-28T02:00:49Z

Hi @qinghaoyang, 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!

oppiabot · 2022-11-28T18:12:08Z

Hi @qinghaoyang, 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!

oppiabot · 2023-01-08T10:08:14Z

Hi @qinghaoyang, 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!

qinghaoyang

@seanlip PTAL.

qinghaoyang · 2022-12-31T01:34:34Z

core/feconf.py

@@ -1638,6 +1638,9 @@ def get_empty_ratings() -> Dict[str, int]:
    SUGGESTION_TYPE_ADD_QUESTION
 ]

+# The sort keys of submitted questions shown on the Contributor Dashboard.
+QUESTIONS_SORT_KEYS = ['Default', 'Date']


qinghaoyang · 2022-12-31T01:43:44Z

...ntributor-dashboard-page/contributions-and-review/contributions-and-review.component.spec.ts

@@ -705,7 +705,7 @@ describe('Contributions and review component', () => {
        expect(component.activeExplorationId).toBeNull();
      }));

-    it('should return true on Review Translations tab', fakeAsync(() => {
+    it('should on Review Translations tab', fakeAsync(() => {


qinghaoyang · 2022-12-31T01:43:49Z

...ntributor-dashboard-page/contributions-and-review/contributions-and-review.component.spec.ts

      expect(alertsService.addSuccessMessage)
        .toHaveBeenCalledWith('Submitted suggestion review.');
    }));

-    it('should return false on Review Questions tab', () => {
+    it('should on Review Questions tab', () => {


qinghaoyang · 2022-12-31T01:59:12Z

...es/contributor-dashboard-page/contributions-and-review/contributions-and-review.component.ts

@@ -515,6 +529,8 @@ export class ContributionsAndReview
    this.TAB_TYPE_CONTRIBUTIONS = 'contributions';
    this.TAB_TYPE_REVIEWS = 'reviews';
    this.TAB_TYPE_ACCOMPLISHMENTS = 'accomplishments';
+    this.QUESTIONS_SORT_KEYS = ['Date'];


Yes. Changed 'Date' to the constant in constants.ts.

qinghaoyang · 2023-01-09T02:21:37Z

...ages/contributor-dashboard-page/services/contribution-and-review-backend-api.service.spec.ts

@@ -74,7 +74,9 @@ describe('Contribution and review backend API service', () => {
      carbas.fetchSuggestionsAsync(
        'SUBMITTED_QUESTION_SUGGESTIONS',
        AppConstants.OPPORTUNITIES_PAGE_SIZE,
-        0, 'All'
+        0,
+        undefined,


Thanks for the suggestion. Updated.

seanlip

Thanks @qinghaoyang! Replied, PTAL.

seanlip · 2023-01-09T04:15:30Z

assets/constants.ts

@@ -5857,7 +5857,8 @@ export default {
    "translation", "voiceover", "question", "submit_question"
  ],

-  "QUESTIONS_SORT_KEY_DATE": "Date",
+  "SUGGESTIONS_SORT_KEY_DATE": "Date",
+  "SUGGESTIONS_SORT_KEY_NULL": "",


Can we not have this constant? This is because Null/None should be different from empty string -- we should actually pass null / None instead.

Also, if possible, I think we shouldn't allow a case where the output is unsorted anyway. If we always require a sort order, it would make things simpler.

I removed this line.

seanlip · 2023-01-09T04:17:38Z

core/templates/pages/contributor-dashboard-page/services/contribution-and-review.service.ts

    return this.fetchSuggestionsAsync(
      this.userCreatedQuestionFetcher,
      shouldResetOffset);
  }

  async getReviewableQuestionSuggestionsAsync(
      shouldResetOffset: boolean = true,
-      sortKey?: string
+      sortKey: string = ''


This should be null if you don't actually have a sort key, rather than ''.

(Better to always have a sort key though, if possible.)

I removed ''

seanlip · 2023-01-09T04:18:49Z

...ntributor-dashboard-page/contributions-and-review/contributions-and-review.component.spec.ts

@@ -768,18 +768,16 @@ describe('Contributions and review component', () => {
      tick();

      expect(component.isReviewTranslationsTab()).toBeTrue();
-      expect(component.isReviewQuestionsTab()).toBeFalse();


I think you can probably keep this assertion, it does no harm and is a reasonable check to make sure the calls are working correctly.

seanlip · 2023-01-09T04:18:54Z

...ntributor-dashboard-page/contributions-and-review/contributions-and-review.component.spec.ts

      spyOn(component, 'openQuestionSuggestionModal').and.callFake(() => {
        return;
      });

      component.switchToTab(component.TAB_TYPE_REVIEWS, 'add_question');
-      expect(component.isReviewTranslationsTab()).toBeFalse();


qinghaoyang

@seanlip PTAL. Currently, the reviewable translations in the backend are not sorted. I added a feature to sort them by date, as I did for reviewable questions. Also, I added a video to show that the reviewable translations are sorted by date by default.

qinghaoyang · 2023-01-09T22:08:24Z

...ntributor-dashboard-page/contributions-and-review/contributions-and-review.component.spec.ts

@@ -768,18 +768,16 @@ describe('Contributions and review component', () => {
      tick();

      expect(component.isReviewTranslationsTab()).toBeTrue();
-      expect(component.isReviewQuestionsTab()).toBeFalse();


qinghaoyang · 2023-01-09T22:08:34Z

...ntributor-dashboard-page/contributions-and-review/contributions-and-review.component.spec.ts

      spyOn(component, 'openQuestionSuggestionModal').and.callFake(() => {
        return;
      });

      component.switchToTab(component.TAB_TYPE_REVIEWS, 'add_question');
-      expect(component.isReviewTranslationsTab()).toBeFalse();


qinghaoyang · 2023-01-09T22:15:58Z

assets/constants.ts

@@ -5857,7 +5857,8 @@ export default {
    "translation", "voiceover", "question", "submit_question"
  ],

-  "QUESTIONS_SORT_KEY_DATE": "Date",
+  "SUGGESTIONS_SORT_KEY_DATE": "Date",
+  "SUGGESTIONS_SORT_KEY_NULL": "",


I removed this line.

qinghaoyang · 2023-01-09T22:16:34Z

core/templates/pages/contributor-dashboard-page/services/contribution-and-review.service.ts

    return this.fetchSuggestionsAsync(
      this.userCreatedQuestionFetcher,
      shouldResetOffset);
  }

  async getReviewableQuestionSuggestionsAsync(
      shouldResetOffset: boolean = true,
-      sortKey?: string
+      sortKey: string = ''


I removed ''

seanlip

Thanks @qinghaoyang! I think this looks great.

Just one question, can you check index.yaml to see if it has the right indexes to support the necessary queries? If not we'll need to add them, otherwise this will throw an error in production when we deploy it.

Also -- @vojtechjelinek I believe index.yaml used to autogenerate indexes, but I think it doesn't do that any more. Is that correct?

qinghaoyang · 2023-01-10T16:30:23Z

@seanlip PTAL.

seanlip

Thanks @qinghaoyang! LGTM!

oppiabot · 2023-01-10T16:36:48Z

Hi @qinghaoyang, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

qinghaoyang added 4 commits November 12, 2022 20:19

Add backend functions for sorting questions with the newest first

0366952

Merge remote-tracking branch 'upstream/develop' into sorting_questions

412fca4

Fix lint errors

5283c47

Completes the covereage test

45fc067

oppiabot bot assigned qinghaoyang Nov 13, 2022

oppiabot bot added PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. PR: don't merge - HAS MERGE CONFLICTS labels Nov 15, 2022

oppiabot bot added the stale label Nov 26, 2022

qinghaoyang added 2 commits November 29, 2022 08:25

Merge remote-tracking branch 'upstream/develop' into sorting_questions

449dc4b

Fix mypy errors

748de42

oppiabot bot removed stale PR: don't merge - HAS MERGE CONFLICTS labels Nov 29, 2022

qinghaoyang added 6 commits December 4, 2022 17:19

Merge remote-tracking branch 'upstream/develop' into sorting_questions

9f470bb

Change backend functions

581281c

Change frontend functions

baa32ff

Fix lint errors

8370912

Fix mypy errors

c33e9d3

Fix arguments error

304673a

oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Dec 6, 2022

qinghaoyang marked this pull request as ready for review December 6, 2022 05:45

qinghaoyang requested a review from a team as a code owner December 6, 2022 05:45

qinghaoyang requested a review from a team December 6, 2022 05:45

qinghaoyang requested a review from a team as a code owner December 6, 2022 05:45

oppiabot bot added stale and removed stale labels Jan 8, 2023

Set sort_key as a mandatory argument

39de584

oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 9, 2023

qinghaoyang commented Jan 9, 2023

View reviewed changes

qinghaoyang requested a review from seanlip January 9, 2023 04:07

qinghaoyang assigned seanlip and unassigned qinghaoyang Jan 9, 2023

Merge remote-tracking branch 'upstream/develop' into sorting_questions

c3fc3b4

seanlip requested changes Jan 9, 2023

View reviewed changes

seanlip assigned qinghaoyang and unassigned seanlip Jan 9, 2023

qinghaoyang added 2 commits January 9, 2023 15:05

Update frontend tests

3f3e83e

Change the default sort key of reviewable translations to date

766a329

qinghaoyang commented Jan 10, 2023

View reviewed changes

qinghaoyang added 2 commits January 9, 2023 18:57

Fix a bug about calculating offset

8d9849a

Fix mypy errors

4e80671

seanlip reviewed Jan 10, 2023

View reviewed changes

Add indexes

428f2ee

qinghaoyang requested a review from a team as a code owner January 10, 2023 16:12

Merge remote-tracking branch 'upstream/develop' into sorting_questions

a7cadb5

qinghaoyang assigned seanlip Jan 10, 2023

seanlip approved these changes Jan 10, 2023

View reviewed changes

seanlip removed their assignment Jan 10, 2023

oppiabot bot added the PR: LGTM label Jan 10, 2023

qinghaoyang merged commit 0065c97 into oppia:develop Jan 10, 2023

vojtechjelinek mentioned this pull request Jan 14, 2023

Fix yaml generation #16921

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #16069: Add a "Newest First" tag for reviewable questions #16534

Fix #16069: Add a "Newest First" tag for reviewable questions #16534

qinghaoyang commented Nov 13, 2022 •

edited

Loading

oppiabot bot commented Nov 13, 2022

oppiabot bot commented Nov 13, 2022

oppiabot bot commented Nov 15, 2022

oppiabot bot commented Nov 19, 2022

oppiabot bot commented Nov 26, 2022

oppiabot bot commented Nov 28, 2022

oppiabot bot commented Nov 28, 2022

oppiabot bot commented Jan 8, 2023

qinghaoyang left a comment

qinghaoyang Dec 31, 2022

qinghaoyang Dec 31, 2022

qinghaoyang Dec 31, 2022

qinghaoyang Dec 31, 2022

qinghaoyang Jan 9, 2023

seanlip left a comment

seanlip Jan 9, 2023

qinghaoyang Jan 9, 2023

seanlip Jan 9, 2023

qinghaoyang Jan 9, 2023

seanlip Jan 9, 2023

qinghaoyang Jan 9, 2023

seanlip Jan 9, 2023

qinghaoyang Jan 9, 2023

qinghaoyang left a comment

qinghaoyang Jan 9, 2023

qinghaoyang Jan 9, 2023

qinghaoyang Jan 9, 2023

qinghaoyang Jan 9, 2023

seanlip left a comment

qinghaoyang commented Jan 10, 2023

seanlip left a comment

oppiabot bot commented Jan 10, 2023

Fix #16069: Add a "Newest First" tag for reviewable questions #16534

Fix #16069: Add a "Newest First" tag for reviewable questions #16534

Conversation

qinghaoyang commented Nov 13, 2022 • edited Loading

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Nov 13, 2022

oppiabot bot commented Nov 13, 2022

oppiabot bot commented Nov 15, 2022

oppiabot bot commented Nov 19, 2022

oppiabot bot commented Nov 26, 2022

oppiabot bot commented Nov 28, 2022

oppiabot bot commented Nov 28, 2022

oppiabot bot commented Jan 8, 2023

qinghaoyang 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

seanlip 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinghaoyang 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

seanlip left a comment

Choose a reason for hiding this comment

qinghaoyang commented Jan 10, 2023

seanlip left a comment

Choose a reason for hiding this comment

oppiabot bot commented Jan 10, 2023

qinghaoyang commented Nov 13, 2022 •

edited

Loading