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

#1792-Enable application details for public institution user #1914

Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
EducationProgramOfferingValidationService,
} from "./services";
import {
ApplicationControllerService,
DesignationAgreementInstitutionsController,
DesignationAgreementControllerService,
InstitutionInstitutionsController,
Expand Down Expand Up @@ -64,6 +65,7 @@ import {
} from "@sims/services/sfas";
import { UserInstitutionsController } from "./route-controllers/user/user.institutions.controller";
import { UserControllerService } from "./route-controllers/user/user.controller.service";
import { ApplicationInstitutionsController } from "./route-controllers/application/application.institutions.controller";

@Module({
imports: [AuthModule],
Expand All @@ -72,6 +74,7 @@ import { UserControllerService } from "./route-controllers/user/user.controller.
InstitutionInstitutionsController,
InstitutionUserInstitutionsController,
InstitutionLocationInstitutionsController,
ApplicationInstitutionsController,
ScholasticStandingInstitutionsController,
ConfirmationOfEnrollmentInstitutionsController,
EducationProgramInstitutionsController,
Expand All @@ -82,6 +85,7 @@ import { UserControllerService } from "./route-controllers/user/user.controller.
OverawardInstitutionsController,
],
providers: [
ApplicationControllerService,
WorkflowClientService,
FormService,
DesignationAgreementService,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import { HttpStatus, INestApplication } from "@nestjs/common";
import * as request from "supertest";
import { DataSource } from "typeorm";
import {
authorizeUserTokenForLocation,
BEARER_AUTH_TYPE,
createTestingAppModule,
getAuthRelatedEntities,
getInstitutionToken,
INSTITUTION_BC_PUBLIC_ERROR_MESSAGE,
INSTITUTION_STUDENT_DATA_ACCESS_ERROR_MESSAGE,
InstitutionTokenTypes,
} from "../../../testHelpers";
import {
createFakeInstitutionLocation,
saveFakeApplication,
} from "@sims/test-utils";
import { Institution, InstitutionLocation } from "@sims/sims-db";

describe("ApplicationInstitutionsController(e2e)-getApplicationDetails", () => {
let app: INestApplication;
let appDataSource: DataSource;
let collegeF: Institution;
let collegeFLocation: InstitutionLocation;
let collegeCLocation: InstitutionLocation;

beforeAll(async () => {
const { nestApplication, dataSource } = await createTestingAppModule();
app = nestApplication;
appDataSource = dataSource;
// College F.
const { institution: collegeF } = await getAuthRelatedEntities(
appDataSource,
InstitutionTokenTypes.CollegeFUser,
);
collegeFLocation = createFakeInstitutionLocation(collegeF);
await authorizeUserTokenForLocation(
appDataSource,
InstitutionTokenTypes.CollegeFUser,
collegeFLocation,
);
// College C.
const { institution: collegeC } = await getAuthRelatedEntities(
appDataSource,
InstitutionTokenTypes.CollegeCUser,
);
collegeCLocation = createFakeInstitutionLocation(collegeC);
await authorizeUserTokenForLocation(
appDataSource,
InstitutionTokenTypes.CollegeCUser,
collegeCLocation,
);
});

it("Should get the student application details when student has a submitted application for the institution.", async () => {
// Arrange
// Create new application.
const savedApplication = await saveFakeApplication(appDataSource, {
institutionLocation: collegeFLocation,
});

const student = savedApplication.student;
const endpoint = `/institutions/application/student/${student.id}/application/${savedApplication.id}`;
const institutionUserToken = await getInstitutionToken(
InstitutionTokenTypes.CollegeFUser,
);

// Act/Assert
await request(app.getHttpServer())
.get(endpoint)
.auth(institutionUserToken, BEARER_AUTH_TYPE)
.expect(HttpStatus.OK)
.expect({
data: {},
id: savedApplication.id,
applicationStatus: savedApplication.applicationStatus,
applicationNumber: savedApplication.applicationNumber,
applicationFormName: "SFAA2022-23",
applicationProgramYearID: savedApplication.programYearId,
});
});

it("Should not have access to get the student application details if the student submitted and application to non-public institution.", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: we're using "Should/when" in the test cases.

// Arrange
// Create new application.
const savedApplication = await saveFakeApplication(appDataSource, {
institutionLocation: collegeCLocation,
});

const student = savedApplication.student;
const endpoint = `/institutions/application/student/${student.id}/application/${savedApplication.id}`;
const institutionUserTokenCUser = await getInstitutionToken(
InstitutionTokenTypes.CollegeCUser,
);

// Act/Assert
await request(app.getHttpServer())
.get(endpoint)
.auth(institutionUserTokenCUser, BEARER_AUTH_TYPE)
.expect(HttpStatus.FORBIDDEN)
.expect({
statusCode: 403,
message: INSTITUTION_BC_PUBLIC_ERROR_MESSAGE,
error: "Forbidden",
});
});

it("Should not get the student application details when application is submitted for different institution.", async () => {
// Arrange
// Create new application.
const savedApplication = await saveFakeApplication(appDataSource, {
institutionLocation: collegeCLocation,
});

const student = savedApplication.student;
const endpoint = `/institutions/application/student/${student.id}/application/${savedApplication.id}`;
const institutionUserToken = await getInstitutionToken(
InstitutionTokenTypes.CollegeFUser,
);

// Act/Assert
await request(app.getHttpServer())
.get(endpoint)
.auth(institutionUserToken, BEARER_AUTH_TYPE)
.expect(HttpStatus.FORBIDDEN)
.expect({
statusCode: 403,
message: INSTITUTION_STUDENT_DATA_ACCESS_ERROR_MESSAGE,
error: "Forbidden",
});
});

afterAll(async () => {
await app?.close();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export class ApplicationAESTController extends BaseController {
await this.applicationControllerService.generateApplicationFormData(
application.data,
);
return this.applicationControllerService.transformToApplicationForAESTDTO(
return this.applicationControllerService.transformToApplicationDTO(
application,
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export class ApplicationControllerService {
* @param application
* @returns Application DTO
*/
async transformToApplicationForAESTDTO(
async transformToApplicationDTO(
application: Application,
): Promise<ApplicationBaseAPIOutDTO> {
return {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { Controller, Get, Param, ParseIntPipe } from "@nestjs/common";
import { ApplicationService } from "../../services";
import BaseController from "../BaseController";
import { ApplicationBaseAPIOutDTO } from "./models/application.dto";
import {
AllowAuthorizedParty,
HasStudentDataAccess,
IsBCPublicInstitution,
UserToken,
} from "../../auth/decorators";
import { ApiTags } from "@nestjs/swagger";
import { ClientTypeBaseRoute } from "../../types";
import { ApplicationControllerService } from "./application.controller.service";
import { AuthorizedParties, IInstitutionUserToken } from "../../auth";

@AllowAuthorizedParty(AuthorizedParties.institution)
@IsBCPublicInstitution()
@Controller("application")
@ApiTags(`${ClientTypeBaseRoute.Institution}-application`)
export class ApplicationInstitutionsController extends BaseController {
constructor(
private readonly applicationService: ApplicationService,
private readonly applicationControllerService: ApplicationControllerService,
) {
super();
}

/**
* API to fetch application details by applicationId.
* This API will be used by institution users.
* @param applicationId for the application.
* @param studentId for the student.
* @returns Application details.
*/
@HasStudentDataAccess("studentId")
@Get("student/:studentId/application/:applicationId")
async getApplication(
@UserToken() userToken: IInstitutionUserToken,
@Param("applicationId", ParseIntPipe) applicationId: number,
@Param("studentId", ParseIntPipe) studentId: number,
): Promise<ApplicationBaseAPIOutDTO> {
const application = await this.applicationService.getApplicationById(
applicationId,
{
loadDynamicData: true,
studentId: studentId,
institutionId: userToken.authorizations.institutionId,
},
);
application.data =
await this.applicationControllerService.generateApplicationFormData(
application.data,
);
return this.applicationControllerService.transformToApplicationDTO(
application,
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export * from "./designation-agreement/designation-agreement.controller.service"
export * from "./designation-agreement/designation-agreement.aest.controller";
export * from "./application/application.aest.controller";
export * from "./application/application.students.controller";
export * from "./application/application.institutions.controller";
export * from "./assessment/assessment.controller.service";
export * from "./institution/institution.aest.controller";
export * from "./institution/institution.institutions.controller";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -513,16 +513,21 @@ export class ApplicationService extends RecordDataModelService<Application> {

/**
* Gets a student application by applicationId.
* Student id can be provided for authorization purposes.
* Student id/ institution id can be provided for authorization purposes.
* @param applicationId application id.
* @param options object that should contain:
* - `loadDynamicData` indicates if the dynamic data(JSONB) should be loaded.
* - `studentId` student id.
* - `institutionId` institution id.
* @returns student application.
*/
async getApplicationById(
applicationId: number,
options?: { loadDynamicData?: boolean; studentId?: number },
options?: {
loadDynamicData?: boolean;
studentId?: number;
institutionId?: number;
Copy link
Collaborator

@dheepak-aot dheepak-aot May 3, 2023

Choose a reason for hiding this comment

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

@guru-aot @andrewsignori-aot @ann-aot @andrepestana-aot @sh16011993

I was wondering that, with so many options in place which justifies the service as such but in this particular scenario where this service is required to be used with studentId and institutionId. Hence from the consumption perspective we need application details of a student who applied to a given institution and IMO we can introduce a service which is more confined to do this by re-using existing service.

like this

async getApplicationByStudentAndInstitution(
    applicationId: number,
    studentId: number,
    institutionId: number,
    options?: { loadDynamicData?: boolean },
  ) {
    return this.getApplicationById(applicationId, {
      studentId,
      institutionId,
      loadDynamicData: options?.loadDynamicData,
    });
  }

This will also mitigate the risk of institution in general. I know in this case decorator is taking care but in general I am saying.

Copy link
Contributor

Choose a reason for hiding this comment

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

what is the benefit here @dheepak-aot? I think I am not getting it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if I am following. Right now the method is consumed 3 times where the only parameter that is required is the applicationId. The idea would be to have more methods where the parameters would be required?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is right @andrewsignori-aot. Not part of this PR. But we can discuss outside about this idea.
@ann-aot It was exactly what @andrewsignori-aot below your comment.

},
): Promise<Application> {
return this.repo.findOne({
select: {
Expand Down Expand Up @@ -578,6 +583,7 @@ export class ApplicationService extends RecordDataModelService<Application> {
student: {
id: options?.studentId,
},
location: { institution: { id: options?.institutionId } },
},
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<template>
<v-navigation-drawer app color="default" permanent>
<v-list
active-class="active-list-item"
density="compact"
bg-color="default"
active-color="primary"
class="no-wrap"
:items="items"
/>
</v-navigation-drawer>
</template>

<script lang="ts">
import { ref, defineComponent } from "vue";
import { InstitutionRoutesConst } from "@/constants/routes/RouteConstants";
import { MenuItemModel } from "@/types";

export default defineComponent({
props: {
studentId: {
type: Number,
required: true,
},
applicationId: {
type: Number,
required: true,
},
},
setup(props) {
const items = ref<MenuItemModel[]>([
{
title: "Student",
props: {
prependIcon: "mdi-school-outline",
to: {
name: InstitutionRoutesConst.STUDENT_APPLICATION_DETAILS,
params: {
applicationId: props.applicationId,
studentId: props.studentId,
},
},
},
},
]);
return {
items,
};
},
});
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export const InstitutionRoutesConst = {
STUDENT_PROFILE: Symbol(),
STUDENT_DETAILS: Symbol(),
STUDENT_APPLICATIONS: Symbol(),
STUDENT_APPLICATION_DETAILS: Symbol(),
STUDENT_RESTRICTIONS: Symbol(),
STUDENT_FILE_UPLOADS: Symbol(),
STUDENT_OVERAWARDS: Symbol(),
Expand Down
3 changes: 2 additions & 1 deletion sources/packages/web/src/router/AESTRoutes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ export const aestRoutes: Array<RouteRecordRaw> = [
},
{
path: AppRoutes.ApplicationDetail,
redirect: { name: AESTRoutesConst.APPLICATION_DETAILS },
props: true,
components: {
default: ApplicationDetails,
Expand All @@ -183,7 +184,7 @@ export const aestRoutes: Array<RouteRecordRaw> = [
},
children: [
{
path: "",
path: AppRoutes.ApplicationView,
name: AESTRoutesConst.APPLICATION_DETAILS,
props: true,
component: StudentApplicationView,
Expand Down
Loading