-
Notifications
You must be signed in to change notification settings - Fork 106
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
[#173171577] Extract front-matter CTA data from message content #1936
Merged
Undermaken
merged 10 commits into
master
from
173171577-extract-frontmatter-data-from-message
Jun 23, 2020
Merged
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
8f061f2
[#173171577] add front-matter lib
Undermaken c558c79
[#173171577] add tests
Undermaken 8678b4f
[#173171577] add ref
Undermaken 3546825
[#173171577] add another test
Undermaken 5f73f4a
[#173171577] fix a typo
Undermaken ea5db98
[#173171577] remove console.log
Undermaken ef24ee8
Merge branch 'master' into 173171577-extract-frontmatter-data-from-me…
Undermaken ae77f58
[#173171577] some improvements
Undermaken 9810235
Merge branch 'master' into 173171577-extract-frontmatter-data-from-me…
Undermaken 0dd7a0f
[#173171577] add fallback on cta extraction
Undermaken File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/** | ||
* this type models 2 cta that could be nested inside the markdown content of a message | ||
* see https://www.pivotaltracker.com/story/show/173171577 | ||
*/ | ||
import * as t from "io-ts"; | ||
|
||
const CTA = t.interface({ | ||
text: t.string, | ||
action: t.string | ||
}); | ||
export type CTA = t.TypeOf<typeof CTA>; | ||
const CTASR = t.interface({ | ||
cta_1: CTA | ||
}); | ||
|
||
const CTASO = t.partial({ | ||
cta_2: CTA | ||
}); | ||
|
||
export const CTAS = t.intersection([CTASR, CTASO], "CTAS"); | ||
|
||
const MessageCTA = t.partial({ | ||
it: CTAS, | ||
en: CTAS | ||
}); | ||
export type CTAS = t.TypeOf<typeof CTAS>; | ||
|
||
export type MessageCTA = t.TypeOf<typeof MessageCTA>; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,210 @@ | ||
import { Option } from "fp-ts/lib/Option"; | ||
import { CreatedMessageWithContent } from "../../../definitions/backend/CreatedMessageWithContent"; | ||
import { FiscalCode } from "../../../definitions/backend/FiscalCode"; | ||
import { MessageBodyMarkdown } from "../../../definitions/backend/MessageBodyMarkdown"; | ||
import { MessageContent } from "../../../definitions/backend/MessageContent"; | ||
import { Timestamp } from "../../../definitions/backend/Timestamp"; | ||
import { CTA, CTAS } from "../../types/MessageCTA"; | ||
import { cleanMarkdownFromCTAs, getCTA, isCtaActionValid } from "../messages"; | ||
|
||
const messageBody = `### this is a message | ||
|
||
this is a body`; | ||
|
||
const CTA_2 = | ||
`--- | ||
it: | ||
cta_1: | ||
text: "premi" | ||
action: "io://PROFILE_MAIN" | ||
cta_2: | ||
text: "premi2" | ||
action: "io://PROFILE_MAIN2" | ||
en: | ||
cta_1: | ||
text: "go1" | ||
action: "io://PROFILE_MAIN" | ||
cta_2: | ||
text: "go2" | ||
action: "io://PROFILE_MAIN2" | ||
--- | ||
` + messageBody; | ||
|
||
const messageWithContent = { | ||
created_at: new Date() as Timestamp, | ||
fiscal_code: "RSSMRA83A12H501D" as FiscalCode, | ||
id: "93726BD8-D29C-48F2-AE6D-2F", | ||
sender_service_id: "dev-service_0", | ||
time_to_live: 3600, | ||
content: { | ||
subject: "Subject - test 1", | ||
markdown: CTA_2, | ||
due_date: new Date() as Timestamp, | ||
payment_data: { | ||
notice_number: "012345678912345678", | ||
amount: 406, | ||
invalid_after_due_date: false | ||
} | ||
} as MessageContent | ||
} as CreatedMessageWithContent; | ||
|
||
describe("getCTA", () => { | ||
it("should have 2 valid CTA", () => { | ||
const maybeCTAs = getCTA(messageWithContent, "it"); | ||
test2CTA( | ||
maybeCTAs, | ||
"premi", | ||
"io://PROFILE_MAIN", | ||
"premi2", | ||
"io://PROFILE_MAIN2" | ||
); | ||
const maybeCTAsEn = getCTA(messageWithContent, "en"); | ||
test2CTA( | ||
maybeCTAsEn, | ||
"go1", | ||
"io://PROFILE_MAIN", | ||
"go2", | ||
"io://PROFILE_MAIN2" | ||
); | ||
}); | ||
|
||
it("should have 1 valid CTA", () => { | ||
const CTA_1 = `--- | ||
it: | ||
cta_1: | ||
text: "premi" | ||
action: "io://PROFILE_MAIN" | ||
--- | ||
some noise`; | ||
|
||
const maybeCTA = getCTA( | ||
{ | ||
...messageWithContent, | ||
content: { | ||
...messageWithContent.content, | ||
markdown: CTA_1 as MessageBodyMarkdown | ||
} | ||
}, | ||
"it" | ||
); | ||
expect(maybeCTA.isSome()).toBeTruthy(); | ||
if (maybeCTA.isSome()) { | ||
const ctas = maybeCTA.value; | ||
expect(ctas.cta_1).toBeDefined(); | ||
expect(ctas.cta_2).not.toBeDefined(); | ||
} | ||
|
||
const maybeCTAEn = getCTA( | ||
{ | ||
...messageWithContent, | ||
content: { | ||
...messageWithContent.content, | ||
markdown: CTA_1 as MessageBodyMarkdown | ||
} | ||
}, | ||
"en" | ||
); | ||
expect(maybeCTAEn.isNone()).toBeTruthy(); | ||
}); | ||
|
||
it("should not have a valid CTA", () => { | ||
const maybeCTA = getCTA( | ||
{ | ||
...messageWithContent, | ||
content: { | ||
...messageWithContent.content, | ||
markdown: "nothing of nothing" as MessageBodyMarkdown | ||
} | ||
}, | ||
"it" | ||
); | ||
expect(maybeCTA.isNone()).toBeTruthy(); | ||
}); | ||
}); | ||
|
||
const test2CTA = ( | ||
maybeCTAS: Option<CTAS>, | ||
text1: string, | ||
action1: string, | ||
text2: string, | ||
action2: string | ||
) => { | ||
expect(maybeCTAS.isSome()).toBeTruthy(); | ||
if (maybeCTAS.isSome()) { | ||
const ctas = maybeCTAS.value; | ||
expect(ctas.cta_1).toBeDefined(); | ||
expect(ctas.cta_2).toBeDefined(); | ||
expect(ctas.cta_1.text).toEqual(text1); | ||
expect(ctas.cta_1.action).toEqual(action1); | ||
if (ctas.cta_2) { | ||
expect(ctas.cta_2.text).toEqual(text2); | ||
expect(ctas.cta_2.action).toEqual(action2); | ||
} | ||
} | ||
}; | ||
|
||
describe("isCtaActionValid", () => { | ||
it("should be a valid internal navigation action", async () => { | ||
const valid: CTA = { text: "dummy", action: "ioit://PROFILE_MAIN" }; | ||
const isValid = await isCtaActionValid(valid); | ||
expect(isValid).toBeTruthy(); | ||
}); | ||
|
||
it("should be not valid (wrong protocol)", async () => { | ||
const invalidProtocol: CTA = { | ||
text: "dummy", | ||
action: "iosit://PROFILE_MAIN" | ||
}; | ||
const isValid = await isCtaActionValid(invalidProtocol); | ||
expect(isValid).toBeFalsy(); | ||
}); | ||
|
||
it("should be not valid (wrong route)", async () => { | ||
const invalidRoute: CTA = { text: "dummy", action: "iosit://WRONG_ROUTE" }; | ||
const isValid = await isCtaActionValid(invalidRoute); | ||
expect(isValid).toBeFalsy(); | ||
}); | ||
|
||
it("should be a valid RN Linking", async () => { | ||
const phoneCtaValid: CTA = { | ||
text: "dummy", | ||
action: "iohandledlink://tel://3471615647" | ||
}; | ||
const isValid = await isCtaActionValid(phoneCtaValid); | ||
expect(isValid).toBeTruthy(); | ||
}); | ||
|
||
it("should be a valid RN Linking (web)", async () => { | ||
const webCtaValid: CTA = { | ||
text: "dummy", | ||
action: "iohandledlink://https://www.google.it" | ||
}; | ||
const isValid = await isCtaActionValid(webCtaValid); | ||
expect(isValid).toBeTruthy(); | ||
}); | ||
}); | ||
|
||
describe("cleanMarkdownFromCTAs", () => { | ||
it("should be the same", async () => { | ||
const markdown = "simple text"; | ||
const cleaned = cleanMarkdownFromCTAs(markdown as MessageBodyMarkdown); | ||
expect(cleaned).toEqual(markdown); | ||
}); | ||
|
||
it("should be cleaned", async () => { | ||
const withCTA = `--- | ||
it: | ||
cta_1: | ||
text: "premi" | ||
action: "io://PROFILE_MAIN" | ||
--- | ||
some noise`; | ||
const cleaned = cleanMarkdownFromCTAs(withCTA as MessageBodyMarkdown); | ||
expect(cleaned).toEqual("some noise"); | ||
}); | ||
|
||
it("should be cleaned (extended version)", async () => { | ||
const cleaned = cleanMarkdownFromCTAs(CTA_2 as MessageBodyMarkdown); | ||
expect(cleaned).toEqual(messageBody); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
In case we are working on a locale that is missing from the CTA we received in the message we return a
none
and we won't show the cta even if it has been added to the message.E.g. User changed to "EN" from preferences option in the message we received just the CTA on IT language -> no CTA displayed. Should we fallback on the received CTA?
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.
@CrisTofani
Yes it could happen.
What if we make it/en as required and not optional?
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.
@Undermaken
I think it could fail anyway cause we don't have the complete control of what we get since it is a remote content, maybe we should handle
attrs
as an array of string and fallback on the first we get?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.
@CrisTofani
starting from your warning I improved the logic and I added a test ae77f58:
now if the front-matter doesn't respect MessageCTA type the decoding will fail.
So if, from remote, we receive a data that doesn't contain it nor en the CTA won't be decoded.
Btw about the fallback we can do as follow: since at the moment the MessageCTA requires it/en, if the CTA for the current locale is not available we can fallback to the other one. if from remote we receive not it/en, fr for example, the decoding will fail
What do you think?
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.
Yep! I find it is a good point!
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.
update within 0dd7a0f