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

feat(contentful): improve error reporting #1470

Merged
merged 2 commits into from
Apr 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,672 changes: 300 additions & 1,372 deletions packages/botonic-plugin-contentful/exports/contentful-export.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion packages/botonic-plugin-contentful/src/cms/callback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export class ResourceId implements ValueObject {
constructor(readonly resourceType: string, readonly id: string) {}

toString(): string {
return `${this.resourceType} with id '${this.id}'`
return `'${this.resourceType}' with id '${this.id}'`
}

equals(other: ContentId): boolean {
Expand Down
4 changes: 2 additions & 2 deletions packages/botonic-plugin-contentful/src/cms/cms-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,12 +266,12 @@ export class ContentfulExceptionWrapper {
content += ` with locale '${context.locale}'`
}
if (resourceId) {
content += ` on '${resourceId.resourceType}' with id '${resourceId.id}'`
content += ` on ${resourceId.toString()}`
}
if (Object.keys(args).length) {
content += ` with args '${JSON.stringify(args)}'`
}
const msg = `Error calling ${this.wrappee}.${method}${content}.`
const msg = `Error calling ${this.wrappee}.${method}${content}`
const exception = new CmsException(msg, contentfulError, resourceId)
const err = this.processError(contentfulError)
this.logger(`${msg} Due to ${err}`)
Expand Down
16 changes: 11 additions & 5 deletions packages/botonic-plugin-contentful/src/cms/exceptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export class CmsException extends Error {
readonly reason?: any,
readonly resourceId?: ResourceId
) {
super(CmsException.mergeMessages(message, reason))
super(CmsException.mergeMessages(message, reason, resourceId))
}

public messageFromReason(): string | undefined {
Expand All @@ -36,13 +36,19 @@ export class CmsException extends Error {
*/
private static mergeMessages(
message: string,
reason: any | undefined
reason: any | undefined,
resourceId: ResourceId | undefined
): string {
if (!reason) {
return message
// resourceId already reported by ErrorReportingCMS, but not yet
// for contents & topContents methods
if (resourceId && !message.includes(resourceId.id)) {
message += ` on content ${resourceId}`
}
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
return `${message} Due to: ${reason.message || String(reason)}`
if (reason) {
message += `. ${reason.message || String(reason)}`
}
return message
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ test('TEST: ErrorReportingCMS content delivery failed', async () => {
.catch((error2: any) => {
expect(error2).toEqual(
new CmsException(
"Error calling CMS.carousel on 'carousel' with id 'id1'.",
"Error calling CMS.carousel on 'carousel' with id 'id1'",
error
)
)
Expand All @@ -41,7 +41,7 @@ test('TEST: ErrorReportingCMS content delivery failed', async () => {
.catch((error2: any) => {
expect(error2).toEqual(
new CmsException(
"Error calling CMS.text with locale 'es' on 'text' with id 'id1'.",
"Error calling CMS.text with locale 'es' on 'text' with id 'id1'",
error
)
)
Expand Down
24 changes: 17 additions & 7 deletions packages/botonic-plugin-contentful/tests/cms/exceptions.test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,24 @@
import { CmsException } from '../../src/cms'
import { CmsException, ContentId, ContentType, ResourceId } from '../../src/cms'

describe('CMSException', () => {
test.each([
['msg1.', new Error('cause error'), 'msg1. Due to: cause error'],
['msg1.', 'non-Error exception', 'msg1. Due to: non-Error exception'],
['msg1', undefined, 'msg1'],
[
'm1',
new Error('cause'),
new ContentId(ContentType.URL, 'ID'),
`m1 on content 'url' with id 'ID'. cause`,
],
['m1', 'non-exception', undefined, 'm1. non-exception'],
['m1', undefined, undefined, 'm1'],
])(
'TEST: CMSException (%s, %s) => %s',
(msg: string, reason: any, expectedToString: string) => {
const sut = new CmsException(msg, reason)
'TEST: CMSException (%s, %s, %s) => %s',
(
msg: string,
reason: any,
resourceId: ResourceId | undefined,
expectedToString: string
) => {
const sut = new CmsException(msg, reason, resourceId)
expect(sut.message).toEqual(expectedToString)
expect(sut.toString()).toEqual(`Error: ${expectedToString}`)
expect(String(sut)).toEqual(`Error: ${expectedToString}`)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ test('TEST: contentful contents buttons', async () => {
locale: 'en',
})
expect(buttons).toSatisfyAll(button => button instanceof Button)
expect(buttons.length).toEqual(32)
expect(buttons.length).toEqual(28)

const star = buttons.filter(b => b.id == '6JYiydi8JhveDAjDSQ2fp4')
expectIs1StarButton(star[0])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ test('TEST: contentful carousel', async () => {
)

// assert
expect(carousel.elements).toHaveLength(3)
expect(carousel.elements).toHaveLength(2)
expect(carousel.common.name).toEqual('INICIO')
expect(carousel.common.keywords).toEqual(['Inicio', 'menu', 'empezar'])
expect(carousel.common.shortText).toEqual('Menú de Inicio')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ test('TEST: contentful text without buttons with carousel followup', async () =>

// assert
expect(text.buttons).toHaveLength(0)
expect((text.common.followUp as cms.Carousel).elements).toHaveLength(3)
expect((text.common.followUp as cms.Carousel).elements).toHaveLength(2)
})

test('TEST: contentful text without buttons with image followup with text followup', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ describe('ManageContentful fields', () => {
expect(e.message).toInclude(
"Error calling ManageCms.updateFields with locale 'en' on 'text' with id '627QkyJrFo3grJryj0vu6L' " +
`with args '${JSON.stringify(newFields)}'. ` +
"Due to: Cannot overwrite field 'text' of entry '627QkyJrFo3grJryj0vu6L'"
"Cannot overwrite field 'text' of entry '627QkyJrFo3grJryj0vu6L'"
)
// eslint-disable-next-line jest/no-try-expect,jest/no-conditional-expect
expect(e.message).toInclude(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ test('TEST: ContentsValidator.validateAllTopContents', async () => {
expect(sut.report.successContents.length).toEqual(
new Set(sut.report.successContents).size
)
expect(sut.report.successContents.length).toBeGreaterThanOrEqual(83)
expect(sut.report.successContents.length).toBeGreaterThanOrEqual(71)
}, 60000)