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

Mkaleem.neslit.10140.email digest #10338

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

KaleemNeslit
Copy link
Collaborator

Link to Issue

Closes: #10140

Description of Changes

  • Updated the Digest email template with dynamic data
  • added the condition at least 3 thread are necessary for sending email
  • updated the Backend response
    For some reason, Liquid doesn't seem to evaluate the variable properly when used with the square bracket notation (communities[name]). Instead, it seems to look for a key literally named test, not the value inside the variable.
    the same code work in different Liquid base editor face the issue with "Knock eidtior"

"How We Fixed It"

  • Updated the Digest email template with dynamic data
  • added the condition at least 3 thread are necessary for sending email
  • updated the Backend response
    For some reason, Liquid doesn't seem to evaluate the variable properly when used with the square bracket notation (communities[name]). Instead, it seems to look for a key literally named test, not the value inside the variable.
    the same code work in different Liquid base editor face the issue with "Knock eidtior"

@KaleemNeslit KaleemNeslit marked this pull request as draft December 17, 2024 16:47
@dillchen dillchen requested a review from timolegros December 17, 2024 16:49
@KaleemNeslit KaleemNeslit marked this pull request as ready for review December 18, 2024 15:36
}),
output: z.object({
threads: z.array(EnrichedThread),
numberOfThreads: z.number(),
Copy link
Contributor

Choose a reason for hiding this comment

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

numberOfThreads seems redundant if same as the length of the threads array

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we need this to add condition in knock . i did not find way to check array length there
image

Copy link
Contributor

@Rotorsoft Rotorsoft left a comment

Choose a reason for hiding this comment

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

just a couple of small details in comments

const sevenDaysAgo = new Date();
sevenDaysAgo.setDate(new Date().getDate() - 7);
body: async ({ payload }) => {
// TODO User payload for unSubscribe once Recap email pr merge
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still a todo? If not plz implement this or create a ticket to followup

return {
threads: threads,
numberOfThreads: threads.length,
// unsubscribe_link: TODO : will add once email recap PR got merged
Copy link
Contributor

Choose a reason for hiding this comment

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

plz link pr number

@@ -17,4 +17,13 @@ export const TriggerNotificationsWorkflow = {
}),
};

export const enableEmailDigest = {
input: z.object({
communityId: z.string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

community_id, plz use snake case as per api convention

}),
output: z.object({
threads: z.array(EnrichedThread),
numberOfThreads: z.number(),
Copy link
Contributor

Choose a reason for hiding this comment

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

snake_case as per api convention

Copy link
Contributor

@mzparacha mzparacha left a comment

Choose a reason for hiding this comment

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

Codewise looks good, didn't test locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Digest Email Template
3 participants