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

Extract table results to a file #193

Merged
merged 31 commits into from
Apr 27, 2023
Merged

Extract table results to a file #193

merged 31 commits into from
Apr 27, 2023

Conversation

Beckyrose200
Copy link
Contributor

@Beckyrose200 Beckyrose200 commented Apr 19, 2023

https://eaflood.atlassian.net/browse/WATER-3964

This pull request is just a small part of a larger project that involves exporting all our database schemas, converting them into CSV files, and uploading them to our Amazon S3 bucket.
Its primary focus is exporting the billing charge categories table to a file. Converting it to a CSV standard first.
To expedite the export process and see the output sooner, we are using a vertical slicing approach, rather than a horizontal one, which means exporting a single table at a time from each schema.

https://eaflood.atlassian.net/jira/software/c/projects/WATER/boards/96?modal=detail&selectedIssue=WATER-3964

This pull request is just a small part of a larger project that involves exporting all our database schemas, converting them into CSV files, and uploading them to our Amazon S3 bucket. Its primary focus is exporting the billing charge categories table to a file. Converting it to a CSV standard first. To expedite the export process and see the output sooner, we are using a vertical slicing approach, rather than a horizontal one, which means exporting a single table at a time from each schema.
@Beckyrose200 Beckyrose200 added the enhancement New feature or request label Apr 19, 2023
@Beckyrose200 Beckyrose200 self-assigned this Apr 19, 2023
@Beckyrose200 Beckyrose200 force-pushed the extract-table-to-file branch from d97deb3 to 47ee54c Compare April 20, 2023 15:55
@Beckyrose200 Beckyrose200 marked this pull request as ready for review April 20, 2023 15:59
@Beckyrose200 Beckyrose200 marked this pull request as draft April 21, 2023 08:52
@Beckyrose200 Beckyrose200 marked this pull request as ready for review April 24, 2023 13:40
Copy link
Member

@Cruikshanks Cruikshanks 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 a big step towards our slice of cake!

I've added some stuff to think about for this PR. I've not gone into the tests because my suggestions will probably result in changes to them.

The top-level takeaway is we just need to review where 'work' is being done, and if there is somewhere else that might be better instead.

Comment on lines 29 to 30
},
temporaryFilePath: process.env.TMPDIR || '/tmp/'
Copy link
Member

Choose a reason for hiding this comment

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

This is typically the location of the tmp directory on a Linux-based system. But clearly, I failed to provide enough clues about the next step to research. 🤦

If OS's have default locations for where they put temporary files, is there a way we can get the OS to tell us where that place is?

There is, and it's built into Node.js

I would opt for using that, rather than having it as a config value we maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted! (I think)

app/services/db-export/convert-to-csv.service.js Outdated Show resolved Hide resolved
app/services/db-export/convert-to-csv.service.js Outdated Show resolved Hide resolved
app/services/db-export/export-data-files.service.js Outdated Show resolved Hide resolved
Copy link
Member

@Cruikshanks Cruikshanks 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 a big step towards our slice of cake!

I've added some stuff to think about for this PR. I've not gone into the tests because my suggestions will probably result in changes to them.

The top-level takeaway is we just need to review where 'work' is being done, and if there is somewhere else that might be better instead.

Copy link
Contributor

@StuAA78 StuAA78 left a comment

Choose a reason for hiding this comment

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

Great stuff! Just a few things to consider. Also a couple of notes for @Cruikshanks as there are a couple of things that could go either way since they're not currently covered by our coding standards.

app/services/db-export/convert-to-csv.service.js Outdated Show resolved Hide resolved
app/services/db-export/convert-to-csv.service.js Outdated Show resolved Hide resolved
test/services/db-export/export-data-files.service.test.js Outdated Show resolved Hide resolved
* @param {Object} columnNames - An object containing the column names.
* @returns {String} - A string representing the formatted header for the column names
*/
function _generateHeader (columnNames) {
Copy link
Contributor

Choose a reason for hiding this comment

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

More a thought than a suggestion so feel free to ignore -- once we've used Object.keys(columnNames) to get an array of column names, we're basically transforming that array into CSV just like we do with each line of the table data. So I don't know if it's worth a refactor to have a general _transformRowToCsv() function that can be used both to generate the header and to convert each line to CSV?

@Beckyrose200 Beckyrose200 force-pushed the extract-table-to-file branch from e91b14f to 31e0911 Compare April 25, 2023 16:18
@Beckyrose200 Beckyrose200 force-pushed the extract-table-to-file branch from 2c4e1a5 to 73ca9ad Compare April 26, 2023 11:29
StuAA78
StuAA78 previously approved these changes Apr 26, 2023
Copy link
Contributor

@StuAA78 StuAA78 left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

I've made 2 suggestions for app/services/db-export/fetch-billing-charge-categories.service.js which will have knock effects on other services and the tests.

So, I've just focused on that service for now. Ping me if you have questions about what I've suggested.

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

I'm not doing it on purpose honest!

Just these 3 then I promise to approve it! 😁 😛

app/services/db-export/convert-to-csv.service.js Outdated Show resolved Hide resolved
app/services/db-export/convert-to-csv.service.js Outdated Show resolved Hide resolved
*/
async function go (data) {
try {
await fs.writeFile(_filenameWithPath('Billing Charge Categories Table Export.csv'), data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't put spaces in a filename. I'd write it more like this: billing_charge_categories_table_export.csv. Personally I think it's a good habit to get into. What do you guys think @Cruikshanks @StuAA78

Sorry @Beckyrose200

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes made, thank you :D

Copy link
Member

Choose a reason for hiding this comment

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

💯

Jozzey
Jozzey previously approved these changes Apr 27, 2023
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

@Beckyrose200 Beckyrose200 merged commit 183e41f into main Apr 27, 2023
@Beckyrose200 Beckyrose200 deleted the extract-table-to-file branch April 27, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants