-
Notifications
You must be signed in to change notification settings - Fork 14
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
#1791 - Enable Assessments tab view and NOA view for Public Institution Users - Part 4 #1976
Conversation
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Outdated
Show resolved
Hide resolved
* @returns disbursement receipt value. | ||
*/ | ||
export function createFakeDisbursementReceiptValue( | ||
initialValues: Partial<DisbursementReceiptValue>, |
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.
Just checking, did the team agree with this?
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 prefer using the options as we've been doing.
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.
From the last PR comment, I believe @guru-aot and myself were good with this approach and @andrewsignori-aot you proposed it.
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.
as discussed, keeping as it is
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.
From the last PR comment, I believe @guru-aot and myself were good with this approach and @andrewsignori-aot you proposed it.
@dheepak-aot to be clear I was asking if the other devs expressed their opinions on that opportunity and we came to a conclusion 😉
We can try this for now and change it along the way if needed.
As I mentioned to @ann-aot and @andrepestana-aot in a previous chat, I am more open to this type of approach for E2E. If it were a production code I would be leaning to have everything explicit as parameters somehow.
sources/packages/backend/libs/test-utils/src/factories/offering-change-request.ts
Show resolved
Hide resolved
sources/packages/backend/libs/test-utils/src/factories/offering-change-request.ts
Outdated
Show resolved
Hide resolved
); | ||
|
||
// Act/Assert | ||
await request(app.getHttpServer()) |
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.
My only concern about this type of test (where no result is expected) is that we need to be really sure that we have the proper data setup in place.
One idea would be calling the method this.educationProgramOfferingService.getOfferingRequestsByApplicationId
to ensure that some data would be returned if the showOfferingChange
was true, but I am not sure about the approach. Thoughts?
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.
One way would be setting up the test to get the details, assert that we get the details, change it in the DB and assert that we don't get the details. This way would be clear that it is because of the changes we did that we are getting a different response.
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.
The point about this endpoint is that the user does not have access to the "Offering Change Requests" at all.
It may work for other situations, but I am not sure if it would work for this specific one.
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.
IMO, this validation could be a part of test case at line 57 to return only valid assessment summary list by adding an offering change to the data and expect API response not have requested offering change.
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, i agree the part of this test case can return the list by adding the offerings in the data arrange.
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.
Please keep in mind that the "Offering Change Requests" are executed by a different query on an isolated if
condition. It is not about one query returning more or fewer records.
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.
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.
as discussed adding ministry endpoint check, to validate the scenario
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.
@ann-aot I am ok with the approach, I am just leaving the comment open for the other DEVs have the opportunity to review also.
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.
Offering Change Requests
@andrewsignori-aot not sure if this response was quoted for my comment. If so I was talking about expecting API result not having offering change in the array result, when there is an offering change for the application.
If not please ignore my comment 😊
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.
Nice work, just minor comments.
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.
Great job. Please take a look at Signori's comments.
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Outdated
Show resolved
Hide resolved
|
||
/** | ||
* Create a fake offering request change. | ||
* @param relations dependencies: |
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.
dependencies.? (ending with period)
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.
@dheepak-aot I really didn't find any issue with the :
or,
or .
when we are expanding the details of a param object in the next lines. we do have some similar comments in our repo. devs let me know your thoughts.
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.
Neither I @ann-aot . It is just that we never had that idea agreed among devs.
It's minor but we can check once now and take it forward.
@andrewsignori-aot @guru-aot @andrepestana-aot @sh16011993 ?
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.
@dheepak-aot I believe our idea (or what we agreed upon) was never to use always .
. it was to use a proper readable English sentence (starting with caps, ,
,.
,:
etc.. whenever wherever needed). Maybe that's the reason, we never had to discuss or agree upon a scenario like this, at least for me, that was my understanding.
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.
To be clear, period(.) is a part of proper readable English. As you mentioned, if we are considering starting with caps, the comment here doesn't start with caps.
Coming to the point, I am not talking about usage of .
or :
. Rather I am talking about the idea of using introduction text for options properties where :
is appropriate for describing options as we do now.
Because that I am seeing both ways 1. @param options
commented with short summary ending with period (instead of introduction ending with :
) and it's properties are commented with it's own description and 2. the other way which is done here. Wanted to follow one way consistently.
I would recommend jumping on to a call and wrap up discussions instead for the benefit of time.
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 am going to assume both the ways work and wrap this point here.
Let us just consider #1976 (comment) further for this 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.
@dheepak-aot I have no idea, why this simple idea/ conversation is going this far. If you still want to discuss this and devs are available we can have a quick sync up, not sure, if it is worth it
@dheepak-aot I believe our idea (or what we agreed upon) was never to use always
.
. it was to use a proper readable English sentence (starting with caps,,
,.
,:
etc.. whenever wherever needed). Maybe that's the reason, we never had to discuss or agree upon a scenario like this, at least for me, that was my understanding.
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.
As I mentioned in above comment, I really don't want to spend team's time about this as it is very trivial. It is just that I am not in same page with what has been discussed.
Great to see Institution student tabs getting close to be done. I have a question regarding the ticket for @JasonCTang Why should institution not see the scholastic standing view request form? Is it because it may be redundant to them as they already have access to this information? |
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.
Please have a look on the other dev comments. Nice work @ann-aot
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.
Thanks for doing the changes, looks good 👍
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.
Great job!
|
||
// Checking the same scenario with the ministry endpoint with a ministry user, to make sure that a fake offering change was created. | ||
// Arrange. | ||
const ministryEndpoint = `/aest/assessment/application/${application.id}/requests`; |
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.
Testing a ministry endpoint at assessment.institutions.controller.*spec
is against the pattern that we have been following in e2e.
I am not able to follow the reason for doing it.
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.
As @andrewsignori-aot mentioned #1976 (comment) and the comment stated in the code, it's not a aest endpoint check, it is just to validate the scenario, whether the offering change was actually created and that's is the reason, the institution endpoint is returning empty list (result without an offering change)
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 am not able to follow @ann-aot . To validate a scenario we are are calling an endpoint here which I feel is an E2E test for an endpoint even though the intention is to validate a scenario.
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.
LGTM, nice work @ann-aot
@dheepak-aot While I was working on ticket, me and @JasonCTang had a discussion on this, this screen is actually used by an institution when a student need any support, sometimes students may be sitting next to the institution while they are viewing these page, so, the idea/requirement for now is to only show whatever a student has access/view in their portal |
Thanks for explaining. I can remember this point now. |
Kudos, SonarCloud Quality Gate passed!
|
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.
Thanks for doing the changes 👍Looks good @ann-aot
1 - Offering change and scholastic change related changes and related e2e
2 - @andrewsignori-aot comment from my previous is addressed, https://github.com/bcgov/SIMS/pull/1952#discussion_r1207267373\
Screenshots:
Institutions view with offering change
2- Ministry view with scholastic standing.
Institutions view scholastic standing,
3- new TC,
Next PR: