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 errors encountered in feature testing. #19100

Merged
merged 40 commits into from
Nov 4, 2023

Conversation

kshitij01042002
Copy link
Contributor

@kshitij01042002 kshitij01042002 commented Nov 2, 2023

Overview

  1. This PR fixes or fixes part of #[fill_in_number_here].

  2. This PR does the following: This PR fixes the API access for the new contributor dashboard.
    This error was recognixed during feature testing.

  3. (For bug-fixing PRs only) The original bug occurred because: Used the API which had access to only super admins, so made the get API access open.

Because of introduction of new roles and new feature needed to update the access according to the feature flag.

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.
  • The linter/Karma presubmit checks have passed on my local machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
  • My PR follows the style guide.
  • 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).

Proof that changes are correct

Proof that question admins can access the old contributor admin dashboard when feature flag is disabled/ can't access the new contributor admin dashboard when feature flag is enabled and question coordinators can access the new contributor admin dashboard when feature flag is enabled.

Screencast.from.03-11-23.11.08.04.AM.IST.webm

No console errors in new contributor admin dashboard and only languages assigned to the coordinator are shown in the dropdown.

Screencast.from.03-11-23.11.12.14.AM.IST.webm

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

@seanlip
Copy link
Member

seanlip commented Nov 3, 2023

@kshitij01042002 Could you please provide more detail in point 3 of the description and also in the proof of changes? Thanks.

@kshitij01042002
Copy link
Contributor Author

Hi @seanlip I have updated the description PTAL. Thanks.

Copy link
Contributor

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

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

Thanks! Just one nit.

Comment on lines 1249 to 1253
if new_dashboard_enabled and (
role_services
.ACTION_ACCESS_NEW_CONTRIBUTOR_DASHBOARD_ADMIN_PAGE in (
self.user.actions
)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if new_dashboard_enabled and (
role_services
.ACTION_ACCESS_NEW_CONTRIBUTOR_DASHBOARD_ADMIN_PAGE in (
self.user.actions
)):
if new_dashboard_enabled and (
role_services.ACTION_ACCESS_NEW_CONTRIBUTOR_DASHBOARD_ADMIN_PAGE
in self.user.actions
):

@vojtechjelinek vojtechjelinek removed their assignment Nov 3, 2023
@seanlip
Copy link
Member

seanlip commented Nov 3, 2023

@kshitij01042002 FYI the changes in #19108 might help fix the backend test failure in your PR. (See the description of that PR for details.)

Copy link
Member

@lkbhitesh07 lkbhitesh07 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @kshitij01042002 LGTM for the code owner file.

@lkbhitesh07 lkbhitesh07 removed their assignment Nov 4, 2023
@oppiabot oppiabot bot added the PR: LGTM label Nov 4, 2023
Copy link

oppiabot bot commented Nov 4, 2023

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

auto-merge was automatically disabled November 4, 2023 10:28

Pull request was closed

@kshitij01042002 kshitij01042002 added this pull request to the merge queue Nov 4, 2023
@seanlip seanlip removed this pull request from the merge queue due to a manual request Nov 4, 2023
@seanlip
Copy link
Member

seanlip commented Nov 4, 2023

I am merging this to unbreak develop.

@seanlip seanlip merged commit 4522db3 into oppia:develop Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants