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

Allow applicant to close a draft application #1202

Merged
merged 4 commits into from May 8, 2019

Conversation

Projects
None yet
3 participants
@luontola
Copy link
Collaborator

commented May 3, 2019

Closes #1186

@luontola luontola force-pushed the applicant-close branch from 82202a2 to bc64363 May 3, 2019

@Macroz
Copy link
Collaborator

left a comment

Where are the tests?

@luontola

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

The command's functionality is tested in rems.workflow.test-dynamic/test-close and who is allowed to run the command is declarative data in rems.application.model/draft-permissions, so any additional tests would only duplicate the declarative data. It's worth writing tests only for stuff that could potentially fail.

@Macroz

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

Well, I think a feature like this should not exist without explicit tests. For example, you should have a test for checking that the permissions model contains the relevant permissions for this feature to be available at correct times, Or even better, you could test as an integration or acceptance test, that an applicant can indeed close a draft application without giving a comment e.g. in test_model.clj and/or test_applications.cljs or elsewhere through API.

@luontola

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

I tested it manually. The API's schema validation for rems.application.commands/CloseCommand and the event schema validation at rems.db.applications/event->json would prevent a nil comment. The thing that could possibly fail in this situation, is that the UI for some reason sends a nil comment instead of empty string when the comment field is not visible. Due to the high cost of end-to-end tests, I don't consider this minor feature worth writing and end-to-end test for it, and we don't really have a way to unit test the UI either. If it gets broken at some point, somebody will write a bug report about it, or with proper monitoring it can be detected when it happens.

@opqdonut
Copy link
Collaborator

left a comment

re: testing, I would like to see an api-level happy-path acceptance test for this feature

Show resolved Hide resolved src/cljs/rems/application.cljs

luontola added some commits May 6, 2019

@luontola luontola requested a review from Macroz May 7, 2019

@luontola luontola merged commit 95051bd into master May 8, 2019

7 checks passed

WIP Ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: doo Your tests passed on CircleCI!
Details
ci/circleci: ok Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: war Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details

@luontola luontola deleted the applicant-close branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.