Skip to content

Fix 403 error popup on page load #2635

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

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

sayomaki
Copy link
Contributor

Description

The PR fixes a bug which shows a 403 error popup every single time a page in the academy tab has been loaded (see image below).

Error popup Error

This bug had likely been introduced in the commit 05cad2b, where src/pages/academy/Academy.ts has a new dispatch effect added upon component loading. Grading overview data should be fetched in grading pages instead of main academy pages.

However, this fix has only been experimentally-tested locally, and further verification is required to ensure no regression towards the grading page(s).

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

After running, ensure the frontend does not make an request to <API>/v2/courses/<course_id>/admin/grading?group=false (in browser console), and that the grading page(s) works normally.

Checklist

  • I have tested this code
  • I have updated the documentation

* Grading overview data should be dispatched in Grading component instead of main Academy component

* Respective code has been moved over accordingly
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5910953297

  • 0 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 37.424%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/pages/academy/grading/Grading.tsx 0 3 0.0%
Totals Coverage Status
Change from base Build 5891888815: -0.004%
Covered Lines: 5739
Relevant Lines: 14415

💛 - Coveralls

@RichDom2185 RichDom2185 requested a review from chownces August 19, 2023 11:35
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@chownces Looks good, but could I just run it through you first? If all is okay, feel free to merge. I'll rebase my other PRs accordingly.

@zhyuhan
Copy link
Contributor

zhyuhan commented Aug 21, 2023

Yup the bug was introduced in the Avenger Dashboard redesign PR (#2437). The fetching of grading overviews was put in the wrong component.

@RichDom2185 I've tested this fix on my side as well and it looks good!

@RichDom2185 RichDom2185 enabled auto-merge (squash) August 21, 2023 06:15
@RichDom2185 RichDom2185 merged commit 963e2e7 into source-academy:master Aug 21, 2023
RichDom2185 pushed a commit to NUS-CS1101S/cadet-frontend that referenced this pull request Aug 25, 2023
* Grading overview data should be dispatched in Grading component instead of main Academy component

* Respective code has been moved over accordingly
RichDom2185 pushed a commit to NUS-CS1101S/cadet-frontend that referenced this pull request Aug 25, 2023
* Grading overview data should be dispatched in Grading component instead of main Academy component

* Respective code has been moved over accordingly
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.

4 participants