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

[CHE-192] Create Filters #139

Conversation

brok3turtl3
Copy link
Contributor

@brok3turtl3 brok3turtl3 commented Jun 10, 2024

Description

This PR adds two basic filters to the main application display page.

Jira Task

CHE-192

Testing Instructions

  1. Make sure your branches are up to date.
  2. Clear all Docker images using npm run docker-remove-all
  3. Spin up dev server on this feature branch using npm run docker-dev
  4. Test functionality. Checking the boxes should filter applications accordingly. New applications show up when the "older than" checkbox is selected but after a couple of minutes disappear as they age out.
  5. Clear all Docker images using npm run docker-remove-all

Checklist

All Team Members

  • I added a descriptive title to this PR.
  • I filled out the Description, Jira Task, and Testing Instructions sections above.
  • I added or updated [Jest unit tests]for any changes to components, server-side controllers, etc.
  • I ran npm run docker-test in my local environment to check that this PR passes all unit tests.
  • I did a quick check to make sure my code changes follow the recommended style guide.

Additional Notes, Images, etc.

  • Delays have been set to appropriate ranges for testing purposes and will be changed for production.

@brok3turtl3 brok3turtl3 added the enhancement New feature or request label Jun 10, 2024
@brok3turtl3 brok3turtl3 self-assigned this Jun 10, 2024
@seantokuzo seantokuzo self-requested a review June 10, 2024 20:40
@brok3turtl3 brok3turtl3 marked this pull request as draft June 10, 2024 21:13
@@ -159,8 +163,8 @@ const CreateApplicationPage = (): JSX.Element => {
className="w-full p-2 rounded bg-gray-800 text-white"
id="date_applied"
name="date_applied"
type="date"
value={formData.date_applied}
type="datetime-local"
Copy link
Contributor

Choose a reason for hiding this comment

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

including the time might be overkill

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely. It is like this so that in the current environment I can test functionally without waiting days. I need the seconds for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good. should we prevent people from choosing a date/time in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we should! New code inc.

Copy link
Contributor

@seantokuzo seantokuzo left a comment

Choose a reason for hiding this comment

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

Left a few comments for review. I can't test this manually, getting an error from the postgres container no PostgreSQL user name specified in startup packet:
Screenshot 2024-06-10 at 1 53 51 PM

Also docker-compose-test.yml is still using the codehammers/ch-dev-dep-v2 image, so running tests locally throws an error that pg package is not installed

@brok3turtl3 brok3turtl3 marked this pull request as ready for review June 10, 2024 23:38
@brok3turtl3 brok3turtl3 requested a review from seantokuzo June 10, 2024 23:38
Copy link
Contributor

@seantokuzo seantokuzo left a comment

Choose a reason for hiding this comment

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

LGTM - this works and builds in future tickets. Only thing I'd say is adding the postgres environment variables for the dev service too, save from having to add them to .env and change the image used in lint and test docker-compose yml's for local testing future tickets for this epic

@brok3turtl3 brok3turtl3 merged commit d5090c2 into CHE-140/epic/Create-Job-Application-Tracker Jun 11, 2024
1 check passed
@brok3turtl3 brok3turtl3 deleted the CHE-192/story/Create-Filters branch June 11, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants