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

Get another user's event attendance #358

Merged
merged 32 commits into from
Dec 31, 2023
Merged

Conversation

maxwn04
Copy link
Contributor

@maxwn04 maxwn04 commented Jul 22, 2023

gets the users past attendance

@github-actions
Copy link

Thanks for contributing! If you've made changes to the API's functionality, please make sure to bump the package version—see this guide to semantic versioning for details—and document those changes as appropriate.

@maxwn04
Copy link
Contributor Author

maxwn04 commented Jul 25, 2023

image

@dowhep dowhep changed the title Max/get user past attendance Get another user's event attendance Jul 26, 2023
Copy link
Collaborator

@shravanhariharan2 shravanhariharan2 left a comment

Choose a reason for hiding this comment

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

Nice work on this first feature! Few comments on the implementation, in general it seems like it's more complicated than it needs to be. Feel free to follow up with any questions

api/controllers/AttendanceController.ts Outdated Show resolved Hide resolved
api/controllers/UserController.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dowhep dowhep left a comment

Choose a reason for hiding this comment

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

quick nit

services/AttendanceService.ts Show resolved Hide resolved
Copy link
Contributor

@dowhep dowhep left a comment

Choose a reason for hiding this comment

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

Really really close to closing, just a couple more nits

@@ -272,4 +272,58 @@ describe('attendance', () => {
expect(attendance.user.uuid).toEqual(staff.uuid);
expect(attendance.event.uuid).toEqual(event.uuid);
});

test('get user attendance by uuid', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename this to 'get another user's attendance by uuid' for clarification

expect(attendancesForEvent).toEqual(expect.arrayContaining(expectedAttendances));
});

test('throws error when canSeeAttendanceFalse', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of canSeeAttendanceFalse, let's update the test description to "throws error when isAttendancePublilc is false"

Copy link
Contributor

@dowhep dowhep left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@nik-dange nik-dange added the Priority High priority issues label Nov 30, 2023
@dowhep dowhep added the Hold Off On Merge Wait to merge pull request until some condition is met label Dec 2, 2023
@dowhep
Copy link
Contributor

dowhep commented Dec 19, 2023

Ok so there are some issues we found on this branch during our test on the staging environment that Nikhil has set up - so far the users cannot see whether they have isAttendancePublic enabled or not, which is important because they cannot toggle the option without knowing the preexisting state.

We should include isAttendancePublic to both the public and the private profile of the users in UserModel and other corresponding places so that the current user and the other users can see whether the isAttendancePublic is true or false.

@dowhep dowhep merged commit 03a4c3b into master Dec 31, 2023
5 checks passed
@dowhep dowhep deleted the Max/get-user-past-attendance branch December 31, 2023 18:29
dowhep added a commit that referenced this pull request Dec 31, 2023
* started migration

* almost completed the query

* change MerchandiseItemModel picture to pictures

* renamed Merchandise to MercahndiseItems

* generated a new model for the pictures

* fixed syntax error with migration

* understand and fix casting error

* edited requests and created some todos

* return position == 0 instead of first picture in array

* migration temp fix

* this one line of code would attach pictures to the collection so that the frontend can display the first picture

* edited some todos to implement a new idea

* edited the service

* created a repository for photos

* Completed the photo create route

* completed the photo deletion route

* getting started with seeding

* make sure the index is consistent

* removed the current file name from url for security purpose

* quick linting

* edited seeding to ensure correctness

* update MerchFactory item for photo support

* refactor and renaming variables

* wrote outline for test and rewrote a method

* fix error

* edits

* removing some junk code

* the error is playing hide and seek with me

* im such a genius

* removing partial debug msgs

* edits

* fixed the order item test

* finished creating tests and pass all tests

* fixed some error

* I CHATGPTED THE SQL AND IT WORKED

* fixed linting error

* edit migration number order

* clean up some unused variables

* renamed picture to uploadedPhoto and photo to merchPhoto for clarity, added some documentation

* remove magic number

* slight seeding edit

* removed position logic

* clarify cascading quetsion

* fixed cascade

* clean up

* clarify seeding data structure

* link fix

* change position in request to string because form data does not accept number

* throw error if position is not a number

* updated deletion logic to delete from s3 first

* link fix

* default url logic fix for positions no longer being 0

* Get another user's event attendance (#358)

* attendences from user uuid

* lint and bugfix

* check same user

* controller factory changes

* lint fixes

* unit test for get attendance by uuid

* lint

* add permision

* add types

* add everything else

* rename migrtion

* test when permision is off

* lint

* forgor to add

* change permission name and fix logic a bit

* rename permission, change patch user

* lint fix

* lint fix

* oops

* check user exists

* lint

* rename tests

* public profile change

* change user model

* lint

* tests

* lint

* updated api version

---------

Co-authored-by: Nikhil Dange <[email protected]>

* staging deployment workflow (#381)

* bumped my migration file number

* bumped my migration file number v2

* remove local settings.json change

* added edge case for migration up

* lint

* lint

---------

Co-authored-by: Max Weng <[email protected]>
Co-authored-by: Nikhil Dange <[email protected]>
Co-authored-by: Nikhil Dange <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hold Off On Merge Wait to merge pull request until some condition is met Priority High priority issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants