-
Notifications
You must be signed in to change notification settings - Fork 31
Conversation
…generate route as a param
…ilding the count query but 0 if we building the fetch data query
saved_search_id: schema.string(), | ||
start: schema.string(), | ||
end: schema.string(), | ||
report_format: schema.oneOf([schema.literal('csv'), schema.literal('xlsx')]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the same 2-space indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use prettier to format the code base. There is also a .prettoerrc
config file in the root path of this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
server/routes/utils/constants.ts
Outdated
pdf = 'pdf', | ||
png = 'png', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the same 2-space indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
server/routes/dataReportMetadata.ts
Outdated
let resp: any = {}; | ||
const ssExist = await axios({ | ||
method: 'GET', | ||
proxy: { host: '127.0.0.1', port: 5601 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think about removing this "check if exists" logic. And handle the non-existing saved search in the failure response of the actual es query via client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I handle it now in the parseEsErrorResponse function
server/routes/dataReportMetadata.ts
Outdated
const report = await context.core.elasticsearch.adminClient.callAsInternalUser( | ||
'index', | ||
{ | ||
index: 'report', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use another index name. Because the reporting landing page needs to query all documents from "report" index to render the reports table view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I changed to datareport. You can also change it later on
// is_count is set to 1 if we building the count query but 0 if we building the fetch data query | ||
export const buildQuery = (report, is_count) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a simple boolean value will make is more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it's a boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the variable "is_count". You are using 0/1 instead of true/false. It's up to you, but I think for a statement "is_count = 1", the readability is not very good.
for (let valueRes of arrayHits) { | ||
for (let data of valueRes.hits) { | ||
const fields = data.fields; | ||
//get all the fields of type date and fromat them to excel format | ||
for (let dateType of report._source.dateFields) { | ||
if (data._source[dateType]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to optimize this 3-layer for loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using 3-layer for loop because of the the Elasticsearch data response structure.
const esb = require('elastic-builder'); | ||
const moment = require('moment'); | ||
const converter = require('json-2-csv'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add dependencies to package.json
, use import .. as .. from..
syntax that typescript can recognize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Can you briefly describe why the client side needs two api calls to get an on-demand csv report? Is it possible to do this through only one api call, by wrapping the |
@@ -0,0 +1,353 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above- 2 space indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two APIs calls are not only for on-demand reports but also for the scheduled reports. Once the user decides to schedule a report, only the metadata will be created. Then when he needs to download, the CSV will be generated from the metadata then return it to the client without saving it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution
rows = nbRows; | ||
nbScroll = Math.floor(nbRows / default_max_size); | ||
} | ||
// let data = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove comments
registerReportRoute(router); | ||
registerReportDefinitionRoute(router); | ||
registerDataReportMetadata(router); | ||
registerDataReport(router); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 space indentation
Issue #49
Added Data report route to generate and save the metadata of a report.
Added another route to tech the data from previously saved metadata with a limit of 150k rows.