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

Feature/admin dashboard event visualization 407 #431

Conversation

cblanken
Copy link
Contributor

Description

Display all current events on Admin Dashboard. The AdminDashboard page now loads all current events and displays them in a list. The page initially loads all the current events automatically, but any subsequent refreshes must use the "Refresh Events" button in the top right.

NOTE: All tests passed for the first two commits cdd0cd7 and cbfaa9f. However, the Admin dashboard cannot currently be accessed while running the development build due to an old /calendar redirect in client/src/contexts/AuthContext/useProvideAuth.js. The latest commit d6db77b removes this redirect for testing the Admin Dashboard, but some adjustments will need to be made to the relevant failing tests.

I've attached test-events.zip with a json file that can be imported into the events Mongo collection to test the loading animation with a large set of events.

Page while loading

together-admin-dashboard1

Page after load

together-admin-dashboard2

Type of change

Please select everything applicable. Please, do not delete any lines.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • This change requires an update to testing

Issue

Checklist:

  • This PR is up to date with the main branch, and merge conflicts have been resolved
  • I have executed npm run test and all tests have passed successfully or I have included details within my PR on the failure.
  • I have executed npm run lint and resolved any outstanding errors. Most issues can be solved by executing npm run format
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@Caleb-Cohen Caleb-Cohen temporarily deployed to DEV July 14, 2023 21:28 — with GitHub Actions Inactive
@Caleb-Cohen
Copy link
Member

Code looks good. Should we consider additional testing?

For example: insert event into DB, does it visualize?

@Caleb-Cohen Caleb-Cohen temporarily deployed to DEV July 18, 2023 20:20 — with GitHub Actions Inactive
Copy link
Member

@Caleb-Cohen Caleb-Cohen left a comment

Choose a reason for hiding this comment

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

Looks good so far, what are your thoughts on deleting the event and checking if it no longer exists?

@cblanken cblanken temporarily deployed to DEV July 26, 2023 23:22 — with GitHub Actions Inactive
@cblanken
Copy link
Contributor Author

Looks good so far, what are your thoughts on deleting the event and checking if it no longer exists?

My thinking is that I should be able to send a DELETE to the /event/:id endpoint and just delete any notes by id. I might add a Cypress command like deleteOwnEvents to make it a bit easier to use.

Then the rendering check can loop through any remaining notes and confirm the note with the target id no longer exists.

I should be able to take a look at it in the next couple days and let you know if I run into any issues with that method.

@Caleb-Cohen
Copy link
Member

Looks good so far, what are your thoughts on deleting the event and checking if it no longer exists?

My thinking is that I should be able to send a DELETE to the /event/:id endpoint and just delete any notes by id. I might add a Cypress command like deleteOwnEvents to make it a bit easier to use.

Then the rendering check can loop through any remaining notes and confirm the note with the target id no longer exists.

I should be able to take a look at it in the next couple days and let you know if I run into any issues with that method.

That's how I would approach it!

- The "Add Events" tests now accepts a list of event obects
- Setup was also refactored into a `before()` and `beforeEach()` for consistency
@cblanken cblanken temporarily deployed to DEV August 1, 2023 19:01 — with GitHub Actions Inactive
@Caleb-Cohen Caleb-Cohen merged commit dada994 into Together-100Devs:main Aug 1, 2023
1 check passed
@Caleb-Cohen
Copy link
Member

LGTM Thanks for taking this on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants