-
Notifications
You must be signed in to change notification settings - Fork 31
Improve logging and error handling; Fix edit report bug; Fix header/footer rendering #123
Improve logging and error handling; Fix edit report bug; Fix header/footer rendering #123
Conversation
…reports into logging
…reports into logging
…reports into logging
…reports into logging
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.
LGTM, one comment left
logger.info('at login page'); | ||
await page.type('[placeholder=Username]', 'admin', { delay: 30 }); | ||
await page.type('[placeholder=Password]', 'admin', { delay: 30 }); | ||
await page.click("[type='submit']"); |
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.
Can we add another goto queryUrl
here after login and delay? Login might not redirect you to the same url
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.
yeah will do
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.
Do we need to delay after clicking submit? or this works instantly
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.
good point. I my latest commit, I removed delay as dependency, instead using page.waitForNavigation()
@@ -51,6 +52,7 @@ export function EditReportDefinition(props) { | |||
}; | |||
|
|||
const reportDefinitionId = props['match']['params']['reportDefinitionId']; | |||
let curReportDefinition: ReportDefinitionSchemaType; |
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.
np: can we spell out the entire word -> currentReportDefinition
vs curReportDefinition
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.
will change
await callUpdateAPI(metadata); | ||
}) | ||
.catch((error) => { | ||
console.log('error when deleting report definition:', error); |
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.
np: add a toast for when this error is triggered
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.
will add
logger.info(`page url includes login? ${page.url().includes('login')}`); | ||
await delay(5000); | ||
|
||
if (page.url().includes('login')) { |
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.
Can you add some comments about what this section is doing? Looks like an auto-login for FGAC domains?
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.
will do
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.
Left some comments
channelSchema | ||
), | ||
}), | ||
delivery: deliverySchema, |
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.
Can the delivery type be inferred from the number of items in the recipients so that in the future (when multiple channels are supported), index need not be migrated to new schema?
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.
That's a good point. Will address in future PR.
logger.info('at login page'); | ||
await page.type('[placeholder=Username]', 'admin', { delay: 30 }); | ||
await page.type('[placeholder=Password]', 'admin', { delay: 30 }); | ||
await page.click("[type='submit']"); |
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.
Do we need to delay after clicking submit? or this works instantly
export const DEFAULT_REPORT_HEADER = '<h1>Open Distro Kibana Reports</h1>'; | ||
|
||
export const DEFAULT_REPORT_FOOTER = '<h1>Open Distro Kibana Reports</h1>'; |
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.
Nice
await page.click("[type='submit']"); | ||
logger.info('Goto queryUrl again after login'); | ||
await page.goto(queryUrl, { waitUntil: 'networkidle0' }); | ||
await delay(5000); |
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.
Why are we hard-coding a delay here?
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.
removed
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.
LGTM- just a few minor questions
Issue #, if available:
Description of changes:
createVisualReport
, (will polish in next PR)statusCode
undefined error in error handler itself, it's a bug.weekly
andmonthly
on UI, since we don't have full support for them now.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.