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 part of #19570: language name in language dropdown list in contributor admin dashboard #19594

Merged
merged 32 commits into from
Feb 20, 2024

Conversation

Aakash-Jakhmola
Copy link
Contributor

@Aakash-Jakhmola Aakash-Jakhmola commented Jan 24, 2024

Overview

  1. This PR fixes part of Fix key release blockers of the contributor admin dashboard #19570.
  2. This PR does the following:
    Start every language name in english first in language drop down.

Essential Checklist

  • The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
  • I have followed the instructions for making a code change.
  • I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I don't have permissions to assign reviewers directly).
  • The linter/Karma presubmit checks pass on my local machine, and my PR follows the coding style guide).
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)

Proof that changes are correct

Screenshot 2024-01-24 at 23 56 47

Proof of changes on desktop with slow/throttled network

NA

Proof of changes on mobile phone

NA

Proof of changes in Arabic language

PR Pointers

Copy link

oppiabot bot commented Jan 24, 2024

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

Copy link

oppiabot bot commented Jan 24, 2024

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

@Aakash-Jakhmola
Copy link
Contributor Author

PTAL @nikitaevg

@nathlobat
Copy link

nathlobat commented Jan 24, 2024

@Aakash-Jakhmola Hi! how are you?
I'm new here!

About this PR, the correction is to put the written languages ​​in the list in English first?
In the image it looks OK, but in https://www.oppiatestserver.org/preferences, I still don't see this change.

If it's because the pr hasn't been approved yet, I'm sorry. So I see that from the image it is correct.

@seanlip

@Aakash-Jakhmola Aakash-Jakhmola changed the title Fix part of #19570: language name in language dropdown list Fix part of #19570: language name in language dropdown list in contributor admin dashboard Jan 25, 2024
@Aakash-Jakhmola Aakash-Jakhmola requested a review from a team as a code owner January 25, 2024 08:05
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.

Thanks @Aakash-Jakhmola -- just a few comments.

@@ -189,6 +189,21 @@ export class ContributorAdminDashboardPageComponent implements OnInit {
);
}

rearrangeLanguageName(language: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

A more concrete description might be putEnglishLanguageNameAtFront()

rearrangeLanguageName(language: string): string {
// Check if the language contains the expected format 'abc (something)'.
if (language.includes('(') && language.includes(')')) {
// Split the language into parts using '(' and ')' as separators
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing all this could you just construct a regex for the desired format and check whether the string matches it? You can use capture groups to collect the relevant parts. I think this would lead to cleaner code and less string manipulation.

Copy link
Contributor Author

@Aakash-Jakhmola Aakash-Jakhmola Jan 26, 2024

Choose a reason for hiding this comment

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

@seanlip I am using RegExp(/([\p{L}]+)\s*\(([\p{L}]+)\)/, 'u') but getting lint error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide the error you see, it's difficult to help without understanding the error you get.

Copy link
Contributor Author

@Aakash-Jakhmola Aakash-Jakhmola Jan 28, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@Aakash-Jakhmola The error message says bad escape \p at position 2 but I'm not sure what's causing that. It seems to work fine when I try it here ... https://esprima.org/demo/validate.html

https://bugs.eclipse.org/bugs/show_bug.cgi?id=492025 suggests that correctness is determine by the "runtime's RegExp impl" but I'm not sure what that's referring to exactly.

I wonder if the issue is that whatever impl is being used doesn't recognize the \p{L}. Could you try replacing that with "accept any character" and see if that helps? Also btw the second part of the regex should just be [A-Za-z ], right?

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.

nikitaevg and others added 13 commits January 26, 2024 10:41
* fix oppia#19511: make name optional for subscribing

* fix lint

* use Optional

* return non_empty validator

* remove Union
…oppia#19402)

* Fix oppia#18661: specific partnership form link

* linting

* frontend tests

* ts-checks

* fixes

* minor changes

* resolve conflict

* fix tests
changes done in about-page.component.css

Co-authored-by: 2008sahil <getinter6@gmail.com>
* Fix Reset Button Styling on Graph

* Change to Flow Root
* added fixed height

* fixed search bar

* lint

---------

Co-authored-by: Nikita Vorobev <vorobyev.nikita@gmail.com>
…19483)

* Trap command used to check for Sudden Exit of running tests on code

* trap command is added with sigint handler

* trap command is added with sigint handler

* trap command is added with sigint handler

* trap command is added with sigint handler

* trap command is added with sigint handler

* changed done as reuqired

* coe updated to check for exit aprt from keyboard interrupt also

* code for all tests updated in makeflie to stop containers in any case of failure

* code for all tests updated in makeflie to stop containers in any case of failure

* make stop will executed for all exit code apart from 0 now

* removed extra spaces

* removed the extra echo commands

* backend tests workflow changed

* backend tests workflow changed

* removed the extra echo commands
)

* fix: chrome version for mac

* chore: removed newlines

* chore: refracted the variables name
…when selecting old ones. (oppia#19549)

* Fix Exploration Editor Category Select

* Change to Value Change
oppia#19565)

* button replaced by primary-button in android page, also disabled attribute added

* fix linter error
* Creates voiceover model

* Adds test for entity voiceover model

* Add voiceover domain and service methods.

* Adds voiceover domain and services tests

* Fixes mypy checks

* Creates voiceover policy model

* Fixes linter issues.

* Address review comments

* Adds regex check for language accent code

* Updates voiceover policy model name.

* Adds voiceover language accent constants

* Adds language accent codes list for voiceovers

* updates docstring

* updates docstring

* Deletes voice policy dir and shifts it into voiceover dir

* Adds helper methods to get & save in VoiceoverAutogenerationPolicyModel

* Creates json files to stores L-a constants.

* Adds test to validate voiceover language accent constants.

* Removes dead code

* Updates code comment

* Updates code comment

* Updates Readme.

* Adds note to developers

* Updates code comment
…9122)

* Front-end changes

* Fix frontend tests

* Fix frontend tests

* Fix linting

* Fix tests

* Fix tests

* Fix tests

* Fix linting

* Fix linting

* Fix linting

* Fix CI checks

* Fix all linting errors

* Update the frontend files and write unit tests

* Fix failing unit tests

* Fix failing unit tests

* Fix failing unit tests

* Fix failing unit tests

* Fix failing CI checks

* Fix failing unit tests

* Fix CI Checks

* Fix lint checks

* Fix typescript typecheck errors

* Fix typescript typecheck errors

* Fix typescript typecheck errors

* Address Review Comments

* Fix flaky tests

* Fix lints and add coverage tests

* Fix lints and add coverage tests
…pia#19588)

searched results alligned to center

Co-authored-by: 2008sahil <getinter6@gmail.com>
Comment on lines 549 to 557
for (const language of component.languageChoices) {
if (language.id === 'hi-en') {
expect(language.language).toBe('Hinglish');
} else if (language.id === 'ms') {
expect(language.language).toBe('Bahasa Melayu (بهاس ملايو)');
} else if (language.id === 'az') {
expect(language.language).toBe('Azerbaijani (Azeri)');
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Generally speaking, if-else statements shouldn't really be used in top-level component tests. It might be better to create a Jasmine spy on AppConstants.SUPPORTED_AUDIO_LANGUAGES that returns our initial test data before arrangement.

Copy link
Contributor Author

@Aakash-Jakhmola Aakash-Jakhmola Feb 7, 2024

Choose a reason for hiding this comment

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

@StephenYu2018 I am not sure how to do this.
I tried this.

spyOnProperty(AppConstants, 'SUPPORTED_AUDIO_LANGUAGES')
      .and.returnValue([
        {
          id: 'ms',
          description: 'بهاس ملايو(Bahasa Melayu)',
          relatedLanguages: ['ms'],
          direction: 'ltr'
        },
        ]);

But it is not working, it gives this error.

Copy link
Member

@seanlip seanlip Feb 8, 2024

Choose a reason for hiding this comment

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

I think maybe what you can do instead is have something along the lines of "assert that languageChoices contains these three dicts". E.g. expect(languageChoices.some(condition)).toBeTrue() where condition checks that the dict matches a target dict.

(Not sure if list.includes() would work, but that would be even better/simpler.)

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. Used toInclude

Comment on lines 253 to 254
component.selectLanguage('Arabic (العربية)');
expect(component.selectedLanguage.language).toBe('Arabic (العربية)');
Copy link
Member

Choose a reason for hiding this comment

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

Can you also do the same thing here as you did in lines 173-180 within this file?

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.

Copy link
Member

@StephenYu2018 StephenYu2018 left a comment

Choose a reason for hiding this comment

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

A few more comments to address before merging.

Copy link

oppiabot bot commented Feb 5, 2024

Unassigning @StephenYu2018 since the review is done.

Copy link

oppiabot bot commented Feb 5, 2024

Hi @Aakash-Jakhmola, it looks like some changes were requested on this pull request by @StephenYu2018. PTAL. Thanks!

@Aakash-Jakhmola
Copy link
Contributor Author

Aakash-Jakhmola commented Feb 9, 2024

Done with changes. PTAL @StephenYu2018. Thanks!

@oppiabot oppiabot bot assigned StephenYu2018 and unassigned Aakash-Jakhmola Feb 9, 2024
Copy link

oppiabot bot commented Feb 9, 2024

Unassigning @Aakash-Jakhmola since a re-review was requested. @Aakash-Jakhmola, please make sure you have addressed all review comments. Thanks!

Copy link
Member

@StephenYu2018 StephenYu2018 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

oppiabot bot commented Feb 11, 2024

Unassigning @StephenYu2018 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Feb 11, 2024
Copy link

oppiabot bot commented Feb 11, 2024

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

Copy link

oppiabot bot commented Feb 14, 2024

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

auto-merge was automatically disabled February 19, 2024 17:42

Head branch was pushed to by a user without write access

@seanlip seanlip added this pull request to the merge queue Feb 20, 2024
Merged via the queue into oppia:develop with commit 6948ba9 Feb 20, 2024
70 checks passed
jayam04 pushed a commit to jayam04/oppia that referenced this pull request Feb 21, 2024
…ontributor admin dashboard (oppia#19594)

* fix language name in language dropdown to list language name first

* change var to let

* fix integration test

* Fix oppia#19511: make name optional for subscribing (oppia#19534)

* fix oppia#19511: make name optional for subscribing

* fix lint

* use Optional

* return non_empty validator

* remove Union

* Fix oppia#18661: Specific partnership interest form for each language (oppia#19402)

* Fix oppia#18661: specific partnership form link

* linting

* frontend tests

* ts-checks

* fixes

* minor changes

* resolve conflict

* fix tests

* Fix issue oppia#19408 About Page is not responsive (oppia#19530)

changes done in about-page.component.css

Co-authored-by: 2008sahil <getinter6@gmail.com>

* Fix oppia#18512: Graph Interaction Mobile Grey Out Bug (oppia#19532)

* Fix Reset Button Styling on Graph

* Change to Flow Root

* Fix oppia#18079: Add fixed height to Skill dialogbox (oppia#19558)

* added fixed height

* fixed search bar

* lint

---------

Co-authored-by: Nikita Vorobev <vorobyev.nikita@gmail.com>

* Fix oppia#19398 Added make stop in case of non zero exit code (oppia#19483)

* Trap command used to check for Sudden Exit of running tests on code

* trap command is added with sigint handler

* trap command is added with sigint handler

* trap command is added with sigint handler

* trap command is added with sigint handler

* trap command is added with sigint handler

* changed done as reuqired

* coe updated to check for exit aprt from keyboard interrupt also

* code for all tests updated in makeflie to stop containers in any case of failure

* code for all tests updated in makeflie to stop containers in any case of failure

* make stop will executed for all exit code apart from 0 now

* removed extra spaces

* removed the extra echo commands

* backend tests workflow changed

* backend tests workflow changed

* removed the extra echo commands

* Fixes google-chrome not found for Mac users in Docker setup (oppia#19512)

* fix: chrome version for mac

* chore: removed newlines

* chore: refracted the variables name

* Fix oppia#19545: Exploration editor category creating new categories when selecting old ones. (oppia#19549)

* Fix Exploration Editor Category Select

* Change to Value Change

* Fix oppia#19536: Adds hover effect to subscribe button on android page (oppia#19565)

* button replaced by primary-button in android page, also disabled attribute added

* fix linter error

* Creates voiceover language policy model (oppia#19456)

* Creates voiceover model

* Adds test for entity voiceover model

* Add voiceover domain and service methods.

* Adds voiceover domain and services tests

* Fixes mypy checks

* Creates voiceover policy model

* Fixes linter issues.

* Address review comments

* Adds regex check for language accent code

* Updates voiceover policy model name.

* Adds voiceover language accent constants

* Adds language accent codes list for voiceovers

* updates docstring

* updates docstring

* Deletes voice policy dir and shifts it into voiceover dir

* Adds helper methods to get & save in VoiceoverAutogenerationPolicyModel

* Creates json files to stores L-a constants.

* Adds test to validate voiceover language accent constants.

* Removes dead code

* Updates code comment

* Updates code comment

* Updates Readme.

* Adds note to developers

* Updates code comment

* GSoC_2.5: Frontend changes to enable pinning of opportunties (oppia#19122)

* Front-end changes

* Fix frontend tests

* Fix frontend tests

* Fix linting

* Fix tests

* Fix tests

* Fix tests

* Fix linting

* Fix linting

* Fix linting

* Fix CI checks

* Fix all linting errors

* Update the frontend files and write unit tests

* Fix failing unit tests

* Fix failing unit tests

* Fix failing unit tests

* Fix failing unit tests

* Fix failing CI checks

* Fix failing unit tests

* Fix CI Checks

* Fix lint checks

* Fix typescript typecheck errors

* Fix typescript typecheck errors

* Fix typescript typecheck errors

* Address Review Comments

* Fix flaky tests

* Fix lints and add coverage tests

* Fix lints and add coverage tests

* Fix oppia#15377: Interaction changes still remain in pop-up after cancellation (oppia#19579)

Fix Customization Args Saved Memento

* Fix issue oppia#19576:Library Page Search Results Alignment Issue  (oppia#19588)

searched results alligned to center

Co-authored-by: 2008sahil <getinter6@gmail.com>

* use regex for matching

* change regex

* Fix oppia#19516: Navigation issues in the blog author page (oppia#19517)

fix: Navigation issues in the blog author page

* fix : [BUG]: Cursor should be hand pointer instead of text pointer oppia#19575 (oppia#19584)

* Click-Arrow edit button text solved

* some minor fixes

* Fix issue oppia#19407:Blog page is not responsive on mobile devices (oppia#19474)

* fixed UI changes in blog page

* passing lint testcase1

* passing lint testcase2

* all lint case passed

* all lint case passed

* final commit

* done suggested changes

* Suggested Changes Done

* added media query at 470px

* fixed UI changes in blog page

* passing lint testcase1

* passing lint testcase2

* all lint case passed

* all lint case passed

* final commit

* done suggested changes

* Suggested Changes Done

* Resolved responsive issues in blog page

* solved responsive issues

* added margin to blog-navigate icon

* fixed all changes

* linter issue resolved

* linter issue resolved

* all linter checks resolved

* changed naming to no-result-found

---------

Co-authored-by: 2008sahil <getinter6@gmail.com>

* FIX oppia#19571 : Add a space after "or" Conjunction and Second Option in Exploration Multi-selection. (oppia#19573)

{{Fix spacing with OR}}

Co-authored-by: Sean Lip <sean@seanlip.org>

* Fix oppia#10647: Fixed faulty navbar on topic editor (oppia#19567)

* fixed the navbar on subtopic page and also added necessary tests

* removed unnecessary function

* fixed coverage issues

* added dynamic icon in navbar

* fixed some linting issues

* simplified the boolean expression

* Fix oppia#19507: Corrected responsive design for exploration editor nav tools  (oppia#19600)

Fixed responsive issue with exploration editor

* Remove unused dependencies: `py` (via `pytest` upgrade)and `PyGithub` (oppia#19553)

* removed unused deps

* removed unused tests

* upgraded pytest to 7.4.4

* fix regex

* fix docstring

* fix tests

* fix test

* fix test

---------

Co-authored-by: Nikita Vorobev <vorobyev.nikita@gmail.com>
Co-authored-by: Yashwardhan Jyani <100014271+yashwardhan-jyani@users.noreply.github.com>
Co-authored-by: Sahil Gupta <155609546+2008sahil@users.noreply.github.com>
Co-authored-by: 2008sahil <getinter6@gmail.com>
Co-authored-by: Justin Nguyen <70992422+jnvtnguyen@users.noreply.github.com>
Co-authored-by: Harsh Keshari <108923011+hrshkshri@users.noreply.github.com>
Co-authored-by: Yash Dugriyal <127002519+yash1378@users.noreply.github.com>
Co-authored-by: Akash Jaiswal <akashjaiswal3846@gmail.com>
Co-authored-by: sagar-subedi <66303548+sagar-subedi@users.noreply.github.com>
Co-authored-by: Nikhil <nikhil.agarwal.2019@gmail.com>
Co-authored-by: Ashwath Kannan <98253080+Ash-2k3@users.noreply.github.com>
Co-authored-by: Afzal Khan <tricktomade@gmail.com>
Co-authored-by: Yogesh Saini <97088265+Ykumar1415@users.noreply.github.com>
Co-authored-by: Rahat <136263179+rahat2134@users.noreply.github.com>
Co-authored-by: Sean Lip <sean@seanlip.org>
Co-authored-by: Sambhav Kaushik <92575005+masterboy376@users.noreply.github.com>
Co-authored-by: amyyeung17 <amyyeung17@gmail.com>
Co-authored-by: Harshvardhan Singh <73544247+Lawful2002@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Affects datastore layer Labels to indicate a PR that changes the datastore layer. PR: LGTM
Projects
None yet
Development

Successfully merging this pull request may close these issues.