Skip to content

Conversation

@koitoror
Copy link
Contributor

@koitoror koitoror commented Sep 10, 2019

What does this PR do?

creates a paginated response of events grouped by the user location

Description of Task to be completed?

  • group paginated events response by the location of the user

How should this be manually tested?

  1. Clone the repo: git clone https://github.com/andela/mrm_api.git
  2. Setup project according to Readme.md
  3. checkout to branch bug/CON-281-fix-activity-page-query
  4. on insomnia to fetch a list of paginated events by user-location send the query
{
 allEvents(startDate: "Mar 21 2017", endDate: "Mar 21 2019", page: 2, perPage: 6) {
   events {
     eventId
     eventTitle
     startTime
     endTime
     room {
       name
       locationId
     }
   }
   pages
   hasPrevious
   hasNext
   queryTotal
 }
}

What are the relevant JIRA stories?

CON-281

Relevant Screenshots

Screenshot 2019-09-11 at 08 55 17

@koitoror koitoror added the wip label Sep 10, 2019
@koitoror koitoror changed the title CON-281-bug(fix-events-query): Fix activity page query CON-281 Fix activity page query Sep 10, 2019
@koitoror koitoror force-pushed the bug/CON-281-fix-activity-page-query branch 3 times, most recently from 0b6d201 to ed544e6 Compare September 10, 2019 08:55
@koitoror koitoror removed the wip label Sep 10, 2019
@koitoror koitoror requested review from cop1fab and mwinel September 10, 2019 09:48
@koitoror koitoror force-pushed the bug/CON-281-fix-activity-page-query branch 2 times, most recently from 2de485e to 32e37a1 Compare September 10, 2019 11:54
Copy link
Contributor

@mnswaleh mnswaleh left a comment

Choose a reason for hiding this comment

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

Nice implementation. However, have you considered that a super Admin user might need to view activities in different locations?

@koitoror koitoror force-pushed the bug/CON-281-fix-activity-page-query branch from 32e37a1 to 85f4449 Compare September 11, 2019 08:49
@koitoror
Copy link
Contributor Author

Nice implementation. However, have you considered that a super Admin user might need to view activities in different locations?

Incorporated it. Thanks.

@koitoror koitoror requested review from mnswaleh and mwinel September 12, 2019 07:05
@koitoror koitoror added To be merged This PR is ready to be merged, developer can start on a new story and removed peer review labels Sep 12, 2019
Copy link
Contributor

@rajeman rajeman left a comment

Choose a reason for hiding this comment

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

@koitoror Kindly change the method you used in implementing this task. Fetching all events from DB and filtering by location is not efficient as these events can be very large. I suggest you do the location filtering at the DB level and in case of Super Admin you check if there is a location argument in the query. If there isn't, then you use the Super Admin's location

@koitoror
Copy link
Contributor Author

@koitoror Kindly change the method you used in implementing this task. Fetching all events from DB and filtering by location is not efficient as these events can be very large. I suggest you do the location filtering at the DB level and in case of Super Admin you check if there is a location argument in the query. If there isn't, then you use the Super Admin's location

Noted

@koitoror koitoror closed this Sep 14, 2019
@koitoror
Copy link
Contributor Author

@koitoror Kindly change the method you used in implementing this task. Fetching all events from DB and filtering by location is not efficient as these events can be very large. I suggest you do the location filtering at the DB level and in case of Super Admin you check if there is a location argument in the query. If there isn't, then you use the Super Admin's location

Noted

@koitoror koitoror reopened this Sep 14, 2019
@koitoror koitoror force-pushed the bug/CON-281-fix-activity-page-query branch 11 times, most recently from d339ff9 to f3ba033 Compare September 16, 2019 20:27
- Add filtering events by current user location
- Add super admin priviledge to view all locations

[Delivers CON-281]
@koitoror koitoror force-pushed the bug/CON-281-fix-activity-page-query branch from f3ba033 to ee3f44b Compare September 17, 2019 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

To be merged This PR is ready to be merged, developer can start on a new story

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants