-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/get detailed door access #265
Conversation
Coverage report
Test suite run failedFailed tests: 2/238. Failed suites: 1/19.
Report generated by 🧪jest coverage report action from f950190 |
I have deployed this PR to feat-get-detailed-door-access-ekorre.review.esek.se 🚀 |
Fix lint and write the changelog. The tests that fail always fail so no worries there (I mean, some worries but separate issue). I'm unsure if the graph you wrote is the most useful solution. It might be more general to create an |
test/integration/access.api.test.ts
Outdated
@@ -197,6 +197,19 @@ describe('setting/getting access for user', () => { | |||
await setGetTest(setAccess(accessMultipleInput), getAccess, expectedAccess); | |||
}); | |||
|
|||
//This seems kinda iffy | |||
it('getting all access users', 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.
might be worth adding a test that adds a individualAccess
and then checks that it actually is returned, that way it keeps it more contained to that specific test
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 mean that I should just check that that one user is present?
Currently 'aa0000bb' "leaks" into the test and if our seeding data ever changes it will break the test.
So do you think it would be reasonable to remove the test checking all users and instead add 2 users; one with and one without individual access. After that check so only 1 of them is returned?
test/integration/access.api.test.ts
Outdated
|
||
expect(accessUsers).toEqual( | ||
expect.arrayContaining( | ||
usernamesToCheck.map((username) => expect.objectContaining({ username })), |
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.
fix the linting error
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 good, added some comments to look into. as @Muncherkin said, update the changelog and version. also fix the linting.
src/api/user.api.ts
Outdated
* @returns All users with individual access | ||
*/ | ||
async getUsersWithIndividualAccess(): Promise< | ||
(PrismaUser & { access: PrismaIndividualAccess[] })[] |
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 update the return type to use the built in prisma types:
Prisma.PrismaUserGetPayload<{
include: {
access: true;
};
}>[]
this way you don't have to explicitly typecast it
src/resolvers/user.resolver.ts
Outdated
@@ -145,6 +145,11 @@ const userResolver: Resolvers = { | |||
return reduce(users, userReduce); | |||
}, | |||
numberOfMembers: async (_, { noAlumni }) => api.getNumberOfMembers(noAlumni === true), | |||
usersWithIndividualAccess: async (_: unknown, __: unknown, ctx: Context) => { |
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.
remove unknown
Update the version in |
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.
resolve the conflicts and you are g2g
I have removed this deploy now 😇 |
Lägger till en function som tar fram alla användare, som har någon typ av individuell access. Är tänkt att användas i frontenden senare för att underlätta för syrelsen att kolla vilka som har access.
Finns ett par problem för tillfället:
(behöver lite feedback :))
Anledningen till att querying behövs är att styrelsen vill veta vilka de har gett individuell access till och vill inte söka igenom alla namn en efter en.