-
Notifications
You must be signed in to change notification settings - Fork 13
#4584 - Pending Offerings queue #5561
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a new "Pending Offerings" queue feature for AEST (ministry) users, allowing them to view and manage education program offerings that require ministry review. The implementation includes a complete full-stack feature with frontend UI, backend API, comprehensive e2e testing, and proper pagination/sorting/search capabilities.
Key Changes:
- Added a new AEST view for pending offerings with search, sort, and pagination capabilities
- Implemented backend API endpoint to retrieve offerings with "Creation Pending" status for active programs
- Added comprehensive e2e tests covering default/custom sorting, search functionality, and inactive program filtering
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ViewPendingOfferings.vue | New Vue component providing the UI for viewing pending offerings with search, sort, and pagination |
| DataTableContract.ts | Added PendingOfferingsHeaders configuration for the data table columns |
| AppRoutes.ts | Added PendingOfferings route definition |
| EducationProgramOffering.dto.ts (web) | Added EducationProgramOfferingPendingAPIOutDTO interface for frontend |
| EducationProgramOfferingApi.ts | Added getPendingOfferings API client method |
| EducationProgramOfferingService.ts | Added getPendingOfferings service method wrapping the API call |
| AESTRoutes.ts | Configured route for ViewPendingOfferings component |
| RouteConstants.ts | Added PENDING_OFFERINGS route constant |
| AESTHomeSideBar.vue | Added "Offerings" menu item linking to pending offerings view |
| education-program-offering.ts (test-utils) | Enhanced factory to support name and submittedDate in initial values |
| datatable-database-mapping-utils.ts | Added sort column mappings for submittedDate, offeringIntensity, and offeringType |
| education-program-offering.service.ts (backend) | Implemented getPendingOfferings business logic with query building, filtering, sorting, and pagination |
| pagination.dto.ts | Extended OfferingsPaginationOptionsAPIInDTO to support additional sort fields |
| education-program-offering.dto.ts (backend) | Added EducationProgramOfferingPendingAPIOutDTO interface |
| education-program-offering.controller.service.ts | Added getPendingOfferings method and removed duplicate service injection |
| education-program-offering.aest.controller.ts | Added GET /pending endpoint with proper decorators and documentation |
| education-program-offering.aest.controller.getPendingOfferings.e2e-spec.ts | Comprehensive e2e tests covering sorting, search, and filtering scenarios |
...ckend/apps/api/src/services/education-program-offering/education-program-offering.service.ts
Outdated
Show resolved
Hide resolved
...ckend/apps/api/src/services/education-program-offering/education-program-offering.service.ts
Outdated
Show resolved
Hide resolved
...ckend/apps/api/src/services/education-program-offering/education-program-offering.service.ts
Outdated
Show resolved
Hide resolved
...ckend/apps/api/src/services/education-program-offering/education-program-offering.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/web/src/views/aest/institution/ViewPendingOfferings.vue
Show resolved
Hide resolved
sources/packages/web/src/views/aest/institution/ViewPendingOfferings.vue
Show resolved
Hide resolved
sources/packages/web/src/views/aest/institution/ViewPendingOfferings.vue
Outdated
Show resolved
Hide resolved
sources/packages/web/src/services/http/EducationProgramOfferingApi.ts
Outdated
Show resolved
Hide resolved
sources/packages/web/src/services/http/EducationProgramOfferingApi.ts
Outdated
Show resolved
Hide resolved
| .auth(token, BEARER_AUTH_TYPE) | ||
| .expect(HttpStatus.OK); | ||
|
|
||
| expect(response.body.count).toBeGreaterThanOrEqual(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @dheepak-aot confirm this but we follow a standard for the expect block, you can find some examples in my PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these goes for all other tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with this approach to avoid blocks of duplicated code in the toEqual assertion. I have simplified the tests that don't return results. I'll look the team's guidance on whether to refactor the other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes same for me, i just got asked to change it to their standard a few times now!
| // Ministry token. | ||
| const token = await getAESTToken(AESTGroups.BusinessAdministrators); | ||
| // Include a search criteria that matches both offerings to avoid test data collisions. | ||
| const endpoint = `/aest/education-program-offering/pending?page=0&pageLimit=10&searchCriteria=Psychology`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can set a base endpoint to avoid duplicating the url many times
| // Ministry token. | ||
| const token = await getAESTToken(AESTGroups.BusinessAdministrators); | ||
|
|
||
| const endpoint = `/aest/education-program-offering/pending?page=0&pageLimit=10&sortField=submittedDate&sortOrder=DESC`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -> Id move the values to a variable and add them to the url using string interpolation, it makes it a bit easier to see what the request is sending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I've done this for sortField/sortOrder/searchCriteria.
| export class OfferingsPaginationOptionsAPIInDTO extends PaginationOptionsAPIInDTO { | ||
| @IsOptional() | ||
| @IsIn(["name"]) | ||
| @IsIn(["name", "submittedDate", "offeringIntensity", "offeringType"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incoming conflict here since i'm also making changes to this in my PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to create a separate DTO. I shouldn't have re-used this.
| relations?.institutionLocation?.institution ?? relations.institution; | ||
| const offering = new EducationProgramOffering(); | ||
| offering.name = faker.lorem.word(); | ||
| offering.name = options?.initialValues?.name ?? faker.lorem.word(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coud have used this in my tests!!
| loading.value = false; | ||
| }; | ||
| onMounted(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think you need a wrapper: onMounted(getofferings); should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's required to wait for the async call to load and seems to be used more often than not in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just a few small comments, it would be nice to have the "study dates" column split into the start/end for consistency, i've done the changes in the offerings view (it also looks really cramped in the table)
|



Overview
For Ministry users, add a new Offerings screen (available through the sidebar) in order to easily see Pending Offerings and navigate to them for approval.
Pending
API
Web
Screenshots
E2E Tests