Conversation
…_PARAMS object for organizing query params throughout codebase (mostly to make it easier to find where a particular query param is used)
…ImplicitConversion for req() inside our api, allowing us to deseralize dates automatically again.
…edProfInvite reason on frontend
…Might be more readable?
…ed to be accepted)
…to the professor selector (to identify them better). Also fixed a bug where the search wasn't working (optionFilterProp was 'label' even though 'label' is a react element. For future devs, just add a custom data to the Select items like I did here).
|
|
||
| Integration tests are located in the `test` folder. | ||
|
|
||
| `yarn test` at root level runs all tests, but you can also selectively run tests by running `yarn test` while inside a package. |
There was a problem hiding this comment.
It took me so long to figure out what this meant and now I feel silly. It's literally just saying "if you run yarn test in packages/frontend it will run frontend tests. if you run yarn test in packages/server it will run backend tests."
We don't have frontend tests, so I just removed it to reduce confusion
| // trying out useSWRImmutable as an experiment. | ||
| // It's basically the same as having 3 useState variables (data, error, isLoading) condensed into one | ||
| // Difference between "useSWR" and "useSWRImmutable" is that this disables all the extra API calls and validation n whatnot that comes loaded with swr | ||
| const { | ||
| data: profInvites, | ||
| error, | ||
| isLoading, | ||
| mutate: mutateProfInvites, | ||
| } = useSWRImmutable( | ||
| `organization/${orgId}/profInvites`, | ||
| async () => await API.profInvites.getAll(orgId), | ||
| ) |
There was a problem hiding this comment.
this ended up working pretty good i think! It will still repeatedly call the backend if it fails, but other than that it's pretty much a condensed version of having 3 state variables. Might try this more in the future
|
|
||
| expect(invite).not.toHaveProperty('organization'); | ||
|
|
||
| expect(res.body).toMatchSnapshot([inviteMatcher]); |
There was a problem hiding this comment.
This is hella nice, certainly gonna be using this more. Makes testing for certain properties way nicer.
Also for what inviteMatcher is: from the code above:
// used when testing with snapshots to say "hey, don't test for exact values for these" since they're gonna be different each time
const inviteMatcher = {
code: expect.any(String),
createdAt: expect.any(String),
expiresAt: expect.any(String),
};So in this instance it's saying "hey, the response body should match this snapshot that is an array with 1 object in it with a code, createdAt, and expiresAt properties"
| OrgRoleChangeReasonMap[ | ||
| history.OrganizationRoleHistory_changeReason | ||
| ], | ||
| changeReason: history.OrganizationRoleHistory_roleChangeReason, |
There was a problem hiding this comment.
this change was needed because the roleChangeReason wasn't actually getting returned to the frontend
| constructor( | ||
| private jwtService: JwtService, | ||
| private configService: ConfigService, | ||
| private profInviteService: ProfInviteService, |
There was a problem hiding this comment.
Just wanted to double check with you @bhunt02 since I know this service isn't normal due to the LTI stuff. Will adding profInviteService (which depends on OrganizationService) as one if its nestjs dependencies break anything with the LTI stuff? It seems to work fine (like there's no nestjs errors) but I just want to make sure I'm not missing something else in another file.
Only mentioning this since it seems enter() wants to pass in courseService rather than have it as a dependency up here.
There was a problem hiding this comment.
IIRC, I made CourseService get optionally passed in to enter() as the LTI stuff does not want courseservice-induced redirects to occur, so this wouldn't be a problem I think? it's just due to cookies that might be hanging around in the courseservice case
There was a problem hiding this comment.
Ah I see leftover cookies does make a compelling case (this sentence sounds like I'm talking about proving the existence of Santa lol).
This does make me want to double check some things, like if i'm clearing the cookie on error cases
… causes the cookies to not be cleared
|
okay finally got around and double-checked to make sure the prof invites were still working after the LTI merge and it seems like it, should be good to review now @bhunt02 |
2 participants
Description
organization-roles.guard.tscan take anorgIdparam (not justoid), and to also throw a 500error if noorgIdwas given. Throws a 400 error if!Number(orgId)Closes #267
Type of change
yarn installHow Has This Been Tested?
Integration tests
Service tests
Modified
expectEmailsSentto allow one to test the email body & subject (and will test to make sure there's no "null" or "undefined" or "object Object" or "{}" in the body & subject.Checklist:
console.logs, leftover unused logic, or anything else that was accidentally committed)