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

fix(#26): add warning if jsonDir contains file when running csv-to-doc command #603

Conversation

sugat009
Copy link
Member

@sugat009 sugat009 commented Jan 24, 2024

Description

Added warning if jsonDir contains files when running csv-to-doc command.

#26

Screenshots

  1. When jsonDir does not exists
    image
  2. When jsonDir exists and user wants to continue after prompt is shown
    image
  3. When jsonDir exists and user does not want to continue after prompt is shown
    image

Code review items

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or integration tests where appropriate
  • Backwards compatible: Works with existing data and configuration. 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.

@sugat009 sugat009 added the Quick win Expected to be an easy fix label Jan 24, 2024
@sugat009 sugat009 requested review from jkuester and m5r January 24, 2024 08:20
@sugat009 sugat009 linked an issue Jan 24, 2024 that may be closed by this pull request
Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

So great! You did an awesome job of paying attention to the patterns and functionality of existing code and matching that with your new additions!

Can you also add two tests to the csv-to-docs.spec.js file, one where the user accepts the prompt and one where the user rejects it? I think you should be able to mock the userPrompt functionality with sinon similar to what is happening in the edit-contacts.spec tests.

src/lib/sync-fs.js Outdated Show resolved Hide resolved
test/lib/sync-fs.spec.js Outdated Show resolved Hide resolved
src/fn/csv-to-docs.js Outdated Show resolved Hide resolved
@jkuester
Copy link
Contributor

Also, I added @tatilepizs to this PR so we can get QA's perspective here too!

@andrablaj
Copy link
Member

andrablaj commented Jan 24, 2024

@jkuester, my bad for not having @tatilepizs added to this PR, and thanks for doing that. @sugat009 asked me about it this morning, and as it was a small change and the PR was already ready for review I figured that it didn't need extra QA support. Thanks for keeping me honest!

@tatilepizs
Copy link

That is fine @andrablaj, and thank you @jkuester 🙂
What I like about those PRs is that I am constantly learning about the development and the feedback

Copy link

@tatilepizs tatilepizs left a comment

Choose a reason for hiding this comment

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

I don't have anything else to add.
Thank you @sugat009

m5r
m5r previously requested changes Jan 25, 2024
Copy link
Member

@m5r m5r left a comment

Choose a reason for hiding this comment

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

Well done on landing your first PR for cht-conf, @jkuester and @tatilepizs covered pretty much everything I had in mind 😄
I added a few comments, let me know if you have any question

src/fn/csv-to-docs.js Outdated Show resolved Hide resolved
test/fn/csv-to-docs.spec.js Outdated Show resolved Hide resolved
test/lib/sync-fs.spec.js Outdated Show resolved Hide resolved
@sugat009 sugat009 requested a review from m5r January 28, 2024 01:14
Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

Sorry, I think when I first looked at this, I was looking at an older version of the code and I did not see that warnIfDirectoryIsNotEmpty was getting exported. (Which probably made at least some of my comments more confusing...)

test/lib/sync-fs.spec.js Outdated Show resolved Hide resolved
src/fn/csv-to-docs.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for the careful revisions! I just had one more minor request for an assertion in the csv-to-docs.spec.js. I am going to approve this PR now, though, meaning that I don't think I need to review how you implemented the changes I requested (this is a common practice at Medic, especially for minor test changes like this. Just helps reduce unnecessary feedback loops since we are all working async). If you do have any questions/concerns with the changes, you can always re-request my review. Otherwise, if everything looks good to you, once you get that assertion added, this PR is good to merge (already have approval from QA)!

You should have permissions to click the merge button. Just make sure it is doing a "squash merge" and the commit message is following the convention. Both of these should be done automatically based on our repo settings (which is why we always merge to main via the PR webpage on GitHub (and not via the CLI or anything) since it will enforce this stuff). The commit message format is very important since merging to master will trigger the semantic-release plugin to try and automatically cut a new release of cht-conf based on the info in the commit message.

test/fn/csv-to-docs.spec.js Show resolved Hide resolved
@jkuester jkuester removed the request for review from m5r February 2, 2024 15:34
@sugat009 sugat009 force-pushed the 26-csv-to-docs-warn-if-json_docs-dir-is-not-empty-when-running branch from 3fa0392 to c8d7134 Compare February 5, 2024 09:39
@sugat009 sugat009 merged commit e445c9b into main Feb 5, 2024
13 checks passed
@sugat009 sugat009 deleted the 26-csv-to-docs-warn-if-json_docs-dir-is-not-empty-when-running branch February 5, 2024 09:47
@medic-ci
Copy link
Collaborator

medic-ci commented Feb 5, 2024

🎉 This PR is included in version 3.21.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Quick win Expected to be an easy fix released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

csv-to-docs: warn if json_docs dir is not empty when running
6 participants