Skip to content

Commit

Permalink
Fix part of #19435: Migrate diagnostic test player page (#19811)
Browse files Browse the repository at this point in the history
* registered with frontend

* removed backend handler for the page

* correspoding tests for the backend handler removed

* removed the link to the old handler

* added access validator class for diagnostic test player

* added url for the validator

* added test class for diagnostic test player

* removed redundant mainpage file

* added service on frontend to call access validators defined on backend

* removed 'downgradeComponent' page component

* corrected constant.ts reference to diagnostic test player, added root page ts file

* added diagnostic test player root template

* removed excess imports and ngbootstrap form dtp module

* added routes within the module

* added the page to the app routing module

* removed the module from webpack config

* fix linter errors

* fixed linter error

* fix linter errorrs

* added frontend tests for root component and validator service method

* fixed url reference to validation service

* fixed pytype check error

* fixed merge conflict

* fixed linter error

* fixed MyPy type checks

* removed unnecessary auth guard for diagnostics test player

* implemented conditional rendering of the page, based on the feature flag value set in backend

* backent test error fixed

* added backend test for DiagnosticTestPlayerAccessValidationHandler

* removed unused import in the module

* correct the component name being tested by the karma test

* added unit test for diagnostic test player root component

* fixed linter errors

* reverted unwanted changes

* linter check fixed

* delegated error handling

* fix linter errors

* modified unit tests to match the code changes in component

* ran prettier for code formatting

* refactored code to include a route guard (instead of error 404 logic inside component)

* minor typo fix

* reverted relave paths to to alias-based paths for consistency
  • Loading branch information
sagar-subedi committed Apr 19, 2024
1 parent 985e84d commit a2eaa0c
Show file tree
Hide file tree
Showing 18 changed files with 405 additions and 195 deletions.
18 changes: 18 additions & 0 deletions assets/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6629,6 +6629,24 @@ export default {
}
]
},
"DIAGNOSTIC_TEST_PLAYER": {
"ROUTE": "diagnostic-test-player",
"TITLE": "Diagnostic Test Player - Oppia",
"META": [
{
"PROPERTY_TYPE": "itemprop",
"PROPERTY_VALUE": "description",
// eslint-disable-next-line max-len
"CONTENT": "With Oppia, you can access free lessons on math, physics, statistics, chemistry, music, history and more from anywhere in the world. Oppia is a nonprofit with the mission of providing high-quality education to those who lack access to it."
},
{
"PROPERTY_TYPE": "itemprop",
"PROPERTY_VALUE": "og:description",
// eslint-disable-next-line max-len
"CONTENT": "With Oppia, you can access free lessons on math, physics, statistics, chemistry, music, history and more from anywhere in the world. Oppia is a nonprofit with the mission of providing high-quality education to those who lack access to it."
}
]
},
"MODERATOR": {
"ROUTE": "moderator",
"TITLE": "Moderator Tools - Oppia",
Expand Down
22 changes: 22 additions & 0 deletions core/controllers/access_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@

from __future__ import annotations

from core import feature_flag_list
from core import feconf
from core.constants import constants
from core.controllers import acl_decorators
from core.controllers import base
from core.domain import blog_services
from core.domain import classroom_config_services
from core.domain import feature_flag_services
from core.domain import learner_group_services
from core.domain import user_services

Expand Down Expand Up @@ -153,6 +155,26 @@ def get(self, username: str) -> None:
raise self.NotFoundException


class DiagnosticTestPlayerAccessValidationHandler(
base.BaseHandler[Dict[str, str], Dict[str, str]]
):
"""Validates access to diagnostic test player page."""

GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

URL_PATH_ARGS_SCHEMAS: Dict[str, str] = {}
HANDLER_ARGS_SCHEMAS: Dict[str, Dict[str, str]] = {'GET': {}}

@acl_decorators.open_access
def get(self) -> None:
"""Handles GET requests."""
if not feature_flag_services.is_feature_flag_enabled(
self.user_id,
feature_flag_list.FeatureNames.DIAGNOSTIC_TEST.value
):
raise self.NotFoundException


class ReleaseCoordinatorAccessValidationHandler(
base.BaseHandler[Dict[str, str], Dict[str, str]]
):
Expand Down
22 changes: 22 additions & 0 deletions core/controllers/access_validators_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,28 @@ def test_release_coordinator_passes_validation(self) -> None:
ACCESS_VALIDATION_HANDLER_PREFIX)


class DiagnosticTestPlayerPageAccessValidationHandlerTests(
test_utils.GenericTestBase):
"""Test for diagnostic test player access validation."""

def test_should_not_access_diagnostic_test_page_when_feature_is_disabled(
self) -> None:
self.get_json(
'%s/can_access_diagnostic_test_player_page' % (
ACCESS_VALIDATION_HANDLER_PREFIX),
expected_status_int=404)

@test_utils.enable_feature_flags(
[feature_flag_list.FeatureNames.DIAGNOSTIC_TEST])
def test_should_access_diagnostic_test_page_when_feature_is_enabled(
self) -> None:
self.get_html_response(
'%s/can_access_diagnostic_test_player_page' % (
ACCESS_VALIDATION_HANDLER_PREFIX),
expected_status_int=200
)


class ProfileExistsValidationHandlerTests(test_utils.GenericTestBase):

def setUp(self) -> None:
Expand Down
22 changes: 0 additions & 22 deletions core/controllers/diagnostic_test_player.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,39 +18,17 @@

import collections

from core import feature_flag_list
from core import feconf
from core.constants import constants
from core.controllers import acl_decorators
from core.controllers import base
from core.domain import feature_flag_services
from core.domain import question_domain
from core.domain import question_services
from core.domain import topic_fetchers

from typing import Dict, List, TypedDict, cast


class DiagnosticTestPlayerPage(
base.BaseHandler[Dict[str, str], Dict[str, str]]
):
"""Renders the diagnostic test player page."""

URL_PATH_ARGS_SCHEMAS: Dict[str, str] = {}
HANDLER_ARGS_SCHEMAS: Dict[str, Dict[str, str]] = {'GET': {}}

@acl_decorators.open_access
def get(self) -> None:
"""Handles GET requests."""
if feature_flag_services.is_feature_flag_enabled(
self.user_id,
feature_flag_list.FeatureNames.DIAGNOSTIC_TEST.value
):
self.render_template('diagnostic-test-player-page.mainpage.html')
else:
raise self.NotFoundException


def normalize_comma_separated_ids(comma_separated_ids: str) -> List[str]:
"""Normalizes a string of comma-separated question IDs into a list of
question IDs.
Expand Down
35 changes: 0 additions & 35 deletions core/controllers/diagnostic_test_player_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@

from __future__ import annotations

from core import feature_flag_list
from core import feconf
from core.domain import platform_parameter_registry
from core.domain import question_services
from core.domain import topic_domain
from core.domain import topic_fetchers
Expand All @@ -27,39 +25,6 @@
from core.tests import test_utils


class DiagnosticTestLandingPageTest(test_utils.GenericTestBase):
"""Test class for the diagnostic test player page."""

def setUp(self) -> None:
super().setUp()
self.signup(self.CURRICULUM_ADMIN_EMAIL, self.CURRICULUM_ADMIN_USERNAME)
self.owner_id = self.get_user_id_from_email(self.CURRICULUM_ADMIN_EMAIL)
self.original_parameter_registry = (
platform_parameter_registry.Registry.parameter_registry.copy())

def tearDown(self) -> None:
super().tearDown()
platform_parameter_registry.Registry.parameter_registry = (
self.original_parameter_registry)

def test_should_not_access_diagnostic_test_page_when_feature_is_disabled(
self) -> None:
self.get_html_response(
feconf.DIAGNOSTIC_TEST_PLAYER_PAGE_URL,
expected_status_int=404
)

@test_utils.enable_feature_flags(
[feature_flag_list.FeatureNames.DIAGNOSTIC_TEST])
def test_should_access_diagnostic_test_page_when_feature_is_enabled(
self
) -> None:
self.get_html_response(
feconf.DIAGNOSTIC_TEST_PLAYER_PAGE_URL,
expected_status_int=200
)


class DiagnosticTestQuestionsHandlerTest(test_utils.GenericTestBase):
"""Test class for the diagnostic test questions handler."""

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
// Copyright 2024 The Oppia Authors. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS-IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
* @fileoverview Tests for DiagnosticTestPlayerPageAuthGuard
*/
import {Location} from '@angular/common';
import {TestBed, fakeAsync, tick} from '@angular/core/testing';
import {
ActivatedRouteSnapshot,
RouterStateSnapshot,
Router,
} from '@angular/router';
import {RouterTestingModule} from '@angular/router/testing';

import {AppConstants} from 'app.constants';
import {DiagnosticTestPlayerPageAuthGuard} from './diagnostic-test-player-page-auth.guard';
import {AccessValidationBackendApiService} from 'pages/oppia-root/routing/access-validation-backend-api.service';

class MockAccessValidationBackendApiService {
validateAccessToDiagnosticTestPlayerPage() {
return Promise.resolve();
}
}

class MockRouter {
navigate(commands: string[]): Promise<boolean> {
return Promise.resolve(true);
}
}

describe('DiagnosticTestPlayerPageAuthGuard', () => {
let guard: DiagnosticTestPlayerPageAuthGuard;
let accessValidationBackendApiService: AccessValidationBackendApiService;
let router: Router;

beforeEach(() => {
TestBed.configureTestingModule({
imports: [RouterTestingModule],
providers: [
DiagnosticTestPlayerPageAuthGuard,
{
provide: AccessValidationBackendApiService,
useClass: MockAccessValidationBackendApiService,
},
{provide: Router, useClass: MockRouter},
Location,
],
});

guard = TestBed.inject(DiagnosticTestPlayerPageAuthGuard);
accessValidationBackendApiService = TestBed.inject(
AccessValidationBackendApiService
);
router = TestBed.inject(Router);
});

it('should allow access if validation succeeds', fakeAsync(() => {
const validateAccessSpy = spyOn(
accessValidationBackendApiService,
'validateAccessToDiagnosticTestPlayerPage'
).and.returnValue(Promise.resolve());
const navigateSpy = spyOn(router, 'navigate').and.returnValue(
Promise.resolve(true)
);

let canActivateResult: boolean | null = null;

guard
.canActivate(new ActivatedRouteSnapshot(), {} as RouterStateSnapshot)
.then(result => {
canActivateResult = result;
});

tick();

expect(canActivateResult).toBeTrue();
expect(validateAccessSpy).toHaveBeenCalled();
expect(navigateSpy).not.toHaveBeenCalled();
}));

it('should redirect to 404 page if validation fails', fakeAsync(() => {
spyOn(
accessValidationBackendApiService,
'validateAccessToDiagnosticTestPlayerPage'
).and.returnValue(Promise.reject());
const navigateSpy = spyOn(router, 'navigate').and.returnValue(
Promise.resolve(true)
);

let canActivateResult: boolean | null = null;

guard
.canActivate(new ActivatedRouteSnapshot(), {} as RouterStateSnapshot)
.then(result => {
canActivateResult = result;
});

tick();

expect(canActivateResult).toBeFalse();
expect(navigateSpy).toHaveBeenCalledWith([
`${AppConstants.PAGES_REGISTERED_WITH_FRONTEND.ERROR.ROUTE}/404`,
]);
}));
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright 2024 The Oppia Authors. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS-IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
* @fileoverview Guard that redirects user to 404 error page
* if the diagnostic test player is not accessible to user.
*/

import {Location} from '@angular/common';
import {Injectable} from '@angular/core';
import {
ActivatedRouteSnapshot,
CanActivate,
Router,
RouterStateSnapshot,
} from '@angular/router';

import {AppConstants} from 'app.constants';
import {AccessValidationBackendApiService} from 'pages/oppia-root/routing/access-validation-backend-api.service';

@Injectable({
providedIn: 'root',
})
export class DiagnosticTestPlayerPageAuthGuard implements CanActivate {
constructor(
private accessValidationBackendApiService: AccessValidationBackendApiService,
private router: Router,
private location: Location
) {}

async canActivate(
route: ActivatedRouteSnapshot,
state: RouterStateSnapshot
): Promise<boolean> {
return new Promise<boolean>(resolve => {
this.accessValidationBackendApiService
.validateAccessToDiagnosticTestPlayerPage()
.then(() => {
resolve(true);
})
.catch(err => {
this.router
.navigate([
`${AppConstants.PAGES_REGISTERED_WITH_FRONTEND.ERROR.ROUTE}/404`,
])
.then(() => {
this.location.replaceState(state.url);
resolve(false);
});
});
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<oppia-angular-root>
<oppia-base-content>
<navbar-breadcrumb>
<ul class="nav navbar-nav oppia-navbar-breadcrumb">
<li>
<span class="oppia-navbar-breadcrumb-separator"></span>
<span>Diagnostic Test Player</span>
</li>
</ul>
</navbar-breadcrumb>

<content>
<oppia-diagnostic-test-player></oppia-diagnostic-test-player>
</content>

<page-footer></page-footer>
</oppia-base-content>
</oppia-angular-root>
Loading

0 comments on commit a2eaa0c

Please sign in to comment.