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 #19435: Migrate Email Dashboard #19627

Merged
merged 24 commits into from
Feb 12, 2024

Conversation

sagar-subedi
Copy link
Collaborator

Overview

  1. This PR fixes part of Migration of Individual Pages from Webpack/AngularJS to Lazy-Loaded Angular Modules #19435.
  2. This PR does the following: Migrates 'Email Dashboard Page' from AngularJs/Webpack to lazy-loaded angular module

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

email-dashboard-migration.mp4

PR Pointers

Copy link

oppiabot bot commented Jan 29, 2024

Hi @sagar-subedi please assign the required reviewer(s) for this PR. Thanks!

@sagar-subedi
Copy link
Collaborator Author

PTAL @DubeySandeep.

Thanks.

@oppiabot oppiabot bot assigned DubeySandeep and unassigned sagar-subedi Jan 29, 2024
Copy link

oppiabot bot commented Jan 29, 2024

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

Copy link
Member

@DubeySandeep DubeySandeep left a comment

Choose a reason for hiding this comment

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

@sagar-subedi This looks great! 😮 I've left one minor comment, PTAL!

@DubeySandeep
Copy link
Member

@sagar-subedi The Backend test is failing on this PR, PTAL!

@DubeySandeep DubeySandeep assigned U8NWXD and unassigned DubeySandeep Jan 29, 2024
@sagar-subedi
Copy link
Collaborator Author

@DubeySandeep Oh, I guess I missed it.
Thanks for reminding.

@sagar-subedi
Copy link
Collaborator Author

PTAL @Nik-09

Thanks.

Copy link
Member

@Nik-09 Nik-09 left a comment

Choose a reason for hiding this comment

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

One small nit.

});
});

it('should not redirect user to 401 page if user is super admin', (done) => {
Copy link
Member

@Nik-09 Nik-09 Feb 10, 2024

Choose a reason for hiding this comment

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

Please rename this test case to something around -- should access the email dashboard page if the user is super admin.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@Nik-09 Nik-09 assigned sagar-subedi and unassigned Nik-09 Feb 10, 2024
Copy link

oppiabot bot commented Feb 11, 2024

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

@sagar-subedi
Copy link
Collaborator Author

@Nik-09 PTAL

Copy link

oppiabot bot commented Feb 12, 2024

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

Copy link

oppiabot bot commented Feb 12, 2024

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

@DubeySandeep
Copy link
Member

DubeySandeep commented Feb 12, 2024

@Nik-09 Can you please review this PR? Also, I would highly suggest reviewing codeowner changes in one go else it delays the PR merge time significantly (Here by 5 days for two minor nit comments each after a week)!

@sagar-subedi Sorry for the trouble, we will try to get the PR merged quickly!

Copy link
Member

@Nik-09 Nik-09 left a comment

Choose a reason for hiding this comment

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

Thanks @sagar-subedi, LGTM

@Nik-09
Copy link
Member

Nik-09 commented Feb 12, 2024

@Nik-09 Can you please review this PR? Also, I would highly suggest reviewing codeowner changes in one go else it delays the PR merge time significantly (Here by 5 days for two minor nit comments each after a week)!

@DubeySandeep Sure, I will take care of this, thank you for your guidance.

@Nik-09 Nik-09 removed their assignment Feb 12, 2024
@DubeySandeep DubeySandeep added this pull request to the merge queue Feb 12, 2024
Merged via the queue into oppia:develop with commit fe15ab8 Feb 12, 2024
70 checks passed
@sagar-subedi
Copy link
Collaborator Author

Thanks @DubeySandeep @Nik-09

Glad to have contributed.

@sagar-subedi sagar-subedi deleted the email-dashboard-migration branch February 12, 2024 17:04
jayam04 pushed a commit to jayam04/oppia that referenced this pull request Feb 21, 2024
* email dashboard page added among the pages registered with frontend

* handler for email dashboard removed from backend

* removed backend test for email dashbard page

* removed link with old handler

* redundant mainpage file deleted

* added auth guard for email dashboard

* removed downgrading of email dashboard page

* added root component for email dashboard

* updated module for email dashboard

* added to the app routing module

* removed email dashboard module from webpack configuration

* fixed linter errors

* linter checks fixed

* navbar added to the email dashboard page

* test added for auth guard and root component of email dashboard

* removed redundant backend test case as the page is migrated to frontend

* added css to the root page component

* style moved to the top to follow the existing standard

* updated 2023 to 2024 in copyright disclaimer

* updated test description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants