-
Notifications
You must be signed in to change notification settings - Fork 7
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
Case insensitive admin search, testing updates and manager setting of notes and report status #281
Conversation
…s before running them
…rend db instances differently
…mespaced services, remove minodata volume.
docker-compose files. Small scripts from here on out
* Review page is now different if the user looking at the page is the approving manager * Manager's review page includes the user's additional note and allows the manager to leave a manager note * Manager note added to DB * Report state is shown in the side nav's "review" section once it leaves "draft" mode * Policy, service and route added to update status of a report
Create separate docker-compose file for running tests, add bin/run-tests command for easy testing
Allow search to be case-insensitive
Conflicts: docs/logical_data_model.md frontend/src/components/Navigator/index.js
Managers can add notes and set status of reports
Increase wait time for the server to spin up in cucummber ci tests
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.
Code looks good, but in my testing there are a couple of issues with the activity report that I'd like to see cleaned up.
-
When the approving manager is a different user than the report creator, the auto-save fails (good! they shouldn't be able to save the report details) and shows a failure message. Please disable autosave when the approving manager is reviewing.
-
I don't think this is related to these changes, but I'm often getting a failure where the save status is not displayed. Looks like it is related to this console error:
* Display save message when creating a new report. * Only users that can write the report send an update request to the server
…r-feedback Document bin/run-tests, make sure frontend tests run in frontend instance
bin/run-tests
Outdated
@@ -16,7 +16,7 @@ docker exec test-backend bash -c "yarn db:seed;" | |||
docker exec test-backend bash -c "yarn test:ci" | |||
|
|||
# Test frontend | |||
docker exec test-backend bash -c "yarn --cwd frontend run test:ci" | |||
docker exec test-frontend bash -c "yarn --cwd frontend run test:ci" |
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.
This doesn't work. Looks like a missing container_name
for the frontend service. --cwd frontend
is also unnecessary (though doesn't break anything) in the frontend container.
That also exposes that this script papers over any failures if you don't read the output carefully. Since this is not used in CI/CD I think I'm ok with that, but I think it'd be good to collect exit statuses and exit the script with non-zero if any individual step exits with non-zero, or just bail out as soon as we get a non-zero.
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.
Hmmm, I've run this a few times and haven't seen this problem. I'll take a look again. Could you get me the output of docker-compose -f docker-compose.test.yml ps --all
on your machine?
As for handing failures, it was intentional that we didn't error out if one of the intermediate steps threw an error. We could catch those statuses, but I'm not sure we want to prevent frontend tests from running if the backend tests fail, for example.
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.
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.
Just a quick check of the doc page for docker-compose exec
, it sounds like that command takes a service name rather than a container name, so that might be related.
While we're discussing, I had originally considered having the frontend and backend tests run as separate scripts, since it seems like engineers might be focused on either frontend or backend during development. Would you be interested in making this accept an optional frontend/backend param?
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.
Would you be interested in making this accept an optional frontend/backend param
If it's quick, sure, but that's pretty low priority to me.
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.
It was quick: https://github.com/adhocteam/Head-Start-TTADP/pull/149/files
I also added some logging of errors and have the script return an exit code value equal to the number of script failures.
Feedback from PR
…s. More echo statements, too
…qual to number of errors that occurred.
Improvements and fixes to bin/run-tests
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.
🎉
commit d1c5786 Merge: 44eeeb8 059425c Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 17:10:20 2021 -0500 Merge pull request #157 from HHS/user-last-login Allow filtering users by last login and access permissions commit 059425c Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 15:25:09 2021 -0500 Make eslint happy commit 09e906f Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 14:46:50 2021 -0500 Refactor user filtering to match access control SOP rules directly commit facfee4 Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 14:03:29 2021 -0500 Allow filtering by only showing users who do have SITE_ACCESS commit b5b93f0 Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 13:40:44 2021 -0500 Allow filtering by last login > 60 or 180 days ago Options to match process here: https://github.com/HHS/Head-Start-TTADP/wiki/Access-Control-&-Account-Management-SOP#account-review-frequency-and-process commit 1d08788 Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 12:18:39 2021 -0500 Display lastLogin on admin user details commit 31883dd Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 11:12:00 2021 -0500 Add lastLogin to user api response commit 60bbefd Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 10:20:03 2021 -0500 Add lastLogin value to user model & update on login commit 1a2dd2c Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 09:36:09 2021 -0500 Prevent validation issues if HSES email updates commit 7554928 Merge: 875f5e2 44eeeb8 Author: Ryan Ahearn <[email protected]> Date: Tue Feb 9 14:15:51 2021 -0500 Merge pull request #286 from adhocteam/main Fix cucumber test on CI commit 875f5e2 Merge: 4bd8566 9f5bb95 Author: Ryan Ahearn <[email protected]> Date: Fri Feb 5 16:31:48 2021 -0500 Merge pull request #284 from adhocteam/main Filter locked users from admin list commit 4bd8566 Merge: 17f9d7f f327246 Author: Ryan Ahearn <[email protected]> Date: Fri Feb 5 16:05:29 2021 -0500 Merge pull request #281 from adhocteam/main Case insensitive admin search, testing updates and manager setting of notes and report status commit 17f9d7f Merge: 16d54e2 8e5d3ef Author: Ryan Ahearn <[email protected]> Date: Wed Feb 3 16:09:21 2021 -0500 Merge pull request #279 from adhocteam/main add frontend for file upload commit 16d54e2 Merge: 541640a e27e7df Author: Ryan Ahearn <[email protected]> Date: Tue Feb 2 15:33:25 2021 -0500 Merge pull request #275 from adhocteam/main Add goal component to frontend commit 541640a Merge: 790ca05 4c3a436 Author: Ryan Ahearn <[email protected]> Date: Fri Jan 29 15:47:13 2021 -0500 Merge pull request #272 from adhocteam/main Save collaborators to report commit 790ca05 Merge: 4948bd3 3cff2f4 Author: Ryan Ahearn <[email protected]> Date: Wed Jan 27 12:04:04 2021 -0500 Merge pull request #267 from adhocteam/main Add file upload api
Improve test output readability
commit f1dbbc1 Merge: bda7c95 728b5c5 Author: Chuck McAndrew <[email protected]> Date: Thu Apr 15 13:39:57 2021 -0400 Merge pull request #281 from adhocteam/cm-improve-readability-of-tests Improve test output readability commit 728b5c5 Merge: 1f6e69f bda7c95 Author: Chuck McAndrew <[email protected]> Date: Thu Apr 15 13:26:18 2021 -0400 Merge branch 'main' into cm-improve-readability-of-tests commit 1f6e69f Author: Chuck McAndrew <[email protected]> Date: Thu Apr 15 09:45:02 2021 -0400 add --silent flag to test by default and add --colors flag
Allow search to be case-insensitive
Description of change
User search is now case-insensitive in the admin page
How to test
Issue(s)
Create separate docker-compose file for running tests, add bin/run-tests command for easy testing
Description of change
While looking at issues with the backend tests, I saw some room for improvement with how docker tests run locally. The
bin/run-tests
file is the culmination of my attempts to sandbox docker-tests from docker-dev environment.I tried multiple docker-compose.yml files, but that had many issues. For one, overrides don't override multivalve options. Another issues was Docker not treating the different configurations as different services, so it wasn't using the test-db when expected.
I looked at the
circleci
CLI, but that only runs jobs, not workflows, and the circleci needed to run tests is two jobs. There may be something useful here, but also introduces another tool.If you look in the commit history, you can also see that I wrote a much longer bash script initially, in part to help me experiment with multiple docker-compose.yml files and in part to compose a number of smallish docker commands into larger actions. When I realized multiple docker-compose.yml files wasn't going to work, this tool became unnecessary. You could retrieve it via
git checkout 8b0beced865b3a9733bb5c61123dcc9d33451981^ -- bin/docker-run
if you're interested.How to test
Run
./bin/run-tests
then start your regular Docker environment. Running tests should not overwrite/modify your development database.Issue(s)
Managers can add notes and set status of reports
Description of change
How to test
CURRENT_USER_ID
is set to "1"Review and submit
section updatesIssue(s)
Checklist