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

Added CSV file support [API, ImportForm] #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zulmehdi
Copy link

@zulmehdi zulmehdi commented Jun 5, 2020

Hi @lazurey,
I was looking for a way to bulk import data into our CMS, and the closest working plugin was yours. Thank you for making it, and saving us some time :)

However, our requirement was to import the data from CSV files, and thus this modification to both the form and the API:

Form

I have allowed the form to accept both .csv and .json files, and added a small "detour" to the form when a CSV file is imported.
Basically, the imported CSV is first converted to an object, and the normal workflow resumes.

API

The API now checks for files passed along the form-data, specifically with "source" filed name, and once it finds it, it converts the CSV or JSON data into object, and replaces toe original request's body with the newly created object. This modification is to allow for the resumption of the original workflow.

I have tried to follow your style as much as possible, modify as little formatting as possible, and have not used any dependency, to have as fewer changes as possible, while adding a powerful feature (at least to me :))

Thought others might use this feature, too.

Please do reach out if there are any issues.

Regards,
Zul

Copy link
Owner

@lazurey lazurey left a comment

Choose a reason for hiding this comment

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

Hi @zul110 , really appreciate your work on it, supporting csv is quite important to a lot of users.
Before I can merge your change, could you check my comments on some of your code, and let me know if they make sense to you.
Also please add test for your code, the current code coverage threshold is 90%, it maybe too high, you can adjust to 80% if you feel there's no sense to reach it.
Thanks a lot.


ctx.request.body = body;

console.log(body);
Copy link
Owner

Choose a reason for hiding this comment

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

Can this be removed?

source,
};

ctx.request.body = body;
Copy link
Owner

Choose a reason for hiding this comment

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

Can we try not to re-assign an object?
Maybe pass the whole context ctx to the service is not a good idea in the beginning, need to refactor the parameters passing to the service, then you can avoid reassigning the body.

const type = ctx.request.files["source"].type;

// Read and parse file['source']
const source = csvHelpers.readFile(
Copy link
Owner

Choose a reason for hiding this comment

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

The csvHelpers could be a fileHelper or something, otherwise it looks weird to make it parse JSON file as well.

return lines;
};

module.exports = {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure whether this file could be moved to utils folder, maybe that's a good place for it. Also please add test cases for it.

@zulmehdi
Copy link
Author

Thanks @lazurey I'll update the request at the earliest, according to your feedback.

Regards,
Zul

@ribeirojose
Copy link

Hey guys, is this still under development? Do we need someone to take over from here? I'd volunteer to do that since this is an important feature for us.

@lazurey
Copy link
Owner

lazurey commented Apr 21, 2021

Hey guys, is this still under development? Do we need someone to take over from here? I'd volunteer to do that since this is an important feature for us.

I think you can go ahead, since it's long time no update.
Or you could fork the repo and add any feature you need.

@nekrasovp
Copy link

Hey guys, is this still under development? Do we need someone to take over from here? I'd volunteer to do that since this is an important feature for us.

How is it going? Did you decide to create a new plugin? I am working on the same issue atm in our project.

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.

4 participants