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

Message in the error logs when xsltproc is not found #7301

Merged
merged 11 commits into from Sep 22, 2021

Conversation

mrsarm
Copy link
Contributor

@mrsarm mrsarm commented Sep 8, 2021

Description

Add clear message in the error logs when the command xsltproc used to process forms is not found.

#6674

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@mrsarm mrsarm marked this pull request as ready for review September 8, 2021 17:06
Copy link
Contributor

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

Nice Job! I added a couple of suggestions

api/src/services/generate-xform.js Outdated Show resolved Hide resolved
api/src/services/generate-xform.js Outdated Show resolved Hide resolved
api/tests/mocha/services/generate-xform.spec.js Outdated Show resolved Hide resolved
api/tests/mocha/services/generate-xform.spec.js Outdated Show resolved Hide resolved
@mrsarm mrsarm force-pushed the 6674-fix-xsltproc-error branch 4 times, most recently from 7856191 to 85871e7 Compare September 9, 2021 22:00
@mrsarm
Copy link
Contributor Author

mrsarm commented Sep 10, 2021

The suggestions were applied. @mrjones-plip could you please review? ( @latin-panda is taking time-off for a few days).

@mrjones-plip
Copy link
Contributor

@mrsarm - Sorry, I didn't get to this today. I'll grab it my Monday if @latin-panda hasn't already grabbed it already on her Monday (my Sunday ;) !

@mrjones-plip
Copy link
Contributor

@mrsarm:

  • sorry 1x b/c I see that @latin-panda is indeed out until later this week. My mistake!
  • sorry 2x b/c I don't think I can tend to this PR well enough in the time given. I kinda escaped learning both node and core well enough to make this as easy as it should be 😅

@mrjones-plip mrjones-plip removed their request for review September 13, 2021 20:16
@mrsarm
Copy link
Contributor Author

mrsarm commented Sep 13, 2021

Don't worry @mrjones-plip , I can wait for @latin-panda return , no urgency . Thanks !

Copy link
Contributor

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

Nice job, thanks for waiting, I left few minor questions in line

api/tests/mocha/services/generate-xform.spec.js Outdated Show resolved Hide resolved
api/src/services/generate-xform.js Outdated Show resolved Hide resolved
Copy link
Contributor

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

Nice LGTM!
It'd be nice to have the other it(....) cases to use the same label style: should do .... when .... so more uniform, otherwise good to go to AT!
Thanks

@mrsarm
Copy link
Contributor Author

mrsarm commented Sep 16, 2021

It'd be nice to have the other it(....) cases to use the same label style: should do .... when .... so more uniform, otherwise good to go to AT!

Needed to re-run CI again because flaky tests so did the grammatical changes as well.

Thanks!

@mrsarm mrsarm merged commit c971293 into master Sep 22, 2021
@mrsarm mrsarm deleted the 6674-fix-xsltproc-error branch September 22, 2021 12:12
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.

None yet

3 participants