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(worker): add defer duration validation #7088

Merged
merged 20 commits into from
Nov 21, 2024

Conversation

djabarovgeorge
Copy link
Contributor

@djabarovgeorge djabarovgeorge commented Nov 20, 2024

What changed? Why was the change needed?

this PR will solve the tickets from #7035
https://linear.app/novu/issue/NV-4739/digest-add-api-restriction-on-the-tier-plan
https://linear.app/novu/issue/NV-4736/delay-add-api-restriction-on-the-tier-plan
This change was made because this PR have a refactoring of the logic that were made in 7035, this way we are having all of the logic in one place.

https://linear.app/novu/issue/NV-4739/digest-add-api-restriction-on-the-tier-plan
https://linear.app/novu/issue/NV-4736/delay-add-api-restriction-on-the-tier-plan

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for novu-stg-vite-dashboard-poc ready!

Name Link
🔨 Latest commit 538e402
🔍 Latest deploy log https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/673f3ba9f651610008016ba6
😎 Deploy Preview https://deploy-preview-7088--novu-stg-vite-dashboard-poc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

pkg-pr-new bot commented Nov 20, 2024

Open in Stackblitz

@novu/client

pnpm add https://pkg.pr.new/novuhq/novu/@novu/client@7088

@novu/framework

pnpm add https://pkg.pr.new/novuhq/novu/@novu/framework@7088

@novu/headless

pnpm add https://pkg.pr.new/novuhq/novu/@novu/headless@7088

@novu/js

pnpm add https://pkg.pr.new/novuhq/novu/@novu/js@7088

@novu/nextjs

pnpm add https://pkg.pr.new/novuhq/novu/@novu/nextjs@7088

@novu/node

pnpm add https://pkg.pr.new/novuhq/novu/@novu/node@7088

@novu/notification-center

pnpm add https://pkg.pr.new/novuhq/novu/@novu/notification-center@7088

novu

pnpm add https://pkg.pr.new/novuhq/novu@7088

@novu/providers

pnpm add https://pkg.pr.new/novuhq/novu/@novu/providers@7088

@novu/react

pnpm add https://pkg.pr.new/novuhq/novu/@novu/react@7088

@novu/react-native

pnpm add https://pkg.pr.new/novuhq/novu/@novu/react-native@7088

@novu/shared

pnpm add https://pkg.pr.new/novuhq/novu/@novu/shared@7088

commit: 538e402

if (Object.keys(issues).length > 0) {
const tierIssues = (issues as Record<string, ContentIssue[]>).tier;
if (tierIssues) {
Logger.warn({ issues: tierIssues, jobId: job._id }, 'Defer duration limit exceeded for job', LOG_CONTEXT);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does Defer mean in the context of a maximum duration limit? The naming is weird to me. I would just go with:

Suggested change
Logger.warn({ issues: tierIssues, jobId: job._id }, 'Defer duration limit exceeded for job', LOG_CONTEXT);
Logger.warn({ issues: tierIssues, jobId: job._id }, 'Maximum duration limit exceeded for job', LOG_CONTEXT);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should stick with this term since it's already used in the 'add job' use case. If needed, I can ensure it's applied consistently across the project, and i will raise it with the team.

We need a clear way to define two things in the project:

  1. The time we delay or notification, without explicitly using 'delay' or 'digest.'
  2. A term for actions that aren’t executed immediately (deferred actions).

Having consistent terms is important because the current lack of clarity causes confusion in the code.

I'm open to using a different term as long as we use it consistently and everyone understands it.

@djabarovgeorge djabarovgeorge changed the base branch from add-api-deferred-action-step-restriction to next November 21, 2024 09:56
Comment on lines +268 to +281
for (const restrictionsError of restrictionsErrors) {
result.amount = [
{
issueType: StepContentIssueEnum.TIER_LIMIT_EXCEEDED,
message: restrictionsError.message,
},
];
result.unit = [
{
issueType: StepContentIssueEnum.TIER_LIMIT_EXCEEDED,
message: restrictionsError.message,
},
];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is the text what we want inside the usecase? maybe keep the usecase result structured, like max_allowed amount , max_allowed_unit and a pass/ fail for it, I would also maybe encapsulate it so it support move validations of similar nature in the future, so like have a nestring with an enum called DEFFER_DURATION

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can encapsulate and refactor it to support other cases when we will have them, for not it will make the code more complex with no reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

my issue is that the usecase is not being really shared, and is just thorwing an exception on the worker no? no point for all the text there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

worker throwing an exception if there is an error, not the new usecase.

Comment on lines +287 to +311
function calculateMilliseconds(amount: number, unit: DigestUnitEnum): number {
switch (unit) {
case DigestUnitEnum.SECONDS:
return amount * 1000;
case DigestUnitEnum.MINUTES:
return amount * 1000 * 60;
case DigestUnitEnum.HOURS:
return amount * 1000 * 60 * 60;
case DigestUnitEnum.DAYS:
return amount * 1000 * 60 * 60 * 24;
case DigestUnitEnum.WEEKS:
return amount * 1000 * 60 * 60 * 24 * 7;
case DigestUnitEnum.MONTHS:
return amount * 1000 * 60 * 60 * 24 * 30; // Using 30 days as an approximation for a month
default:
return 0;
}
}

function isValidDigestUnit(unit: unknown): unit is DigestUnitEnum {
return Object.values(DigestUnitEnum).includes(unit as DigestUnitEnum);
}

function isNumber(value: unknown): value is number {
return !Number.isNaN(Number(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

would expect it inside the use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not at the current use case structure. at the moment the use case expects the computed amount, and then it applies the product restriction on top of the data.

Copy link
Contributor

Choose a reason for hiding this comment

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

can't we change it? there is a worker method computing this also, lets merge this logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as i said worker logic is more complex and fragile i would not touch it as part of this work.

return {
name: 'Digest Test Step',
type: StepTypeEnum.DIGEST,
...overrides,
Copy link
Contributor

Choose a reason for hiding this comment

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

what would you add here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

controlValue

Copy link
Contributor

Choose a reason for hiding this comment

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

should not be used, use patch instead

@@ -1062,10 +1092,11 @@ function buildEmailStep(): StepCreateDto {
type: StepTypeEnum.EMAIL,
};
}
function buildDigestStep(): StepCreateDto {
function buildDigestStep(overrides: Partial<StepCreateDto> = {}): StepCreateDto {
Copy link
Contributor

Choose a reason for hiding this comment

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

control values should not be updated from here anymore, we have a patch step, use it, I'm going to delete this functionality soon

})
);

throw new Error(DetailEnum.DEFER_DURATION_LIMIT_EXCEEDED);
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this lacking some info, why, by how much and so on?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a specific error class for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a "safe-guard" error throw as the executionLogRoute will throw the same error inside of it. meaning we will never get to this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would make it more clear that if we got here we have a bug, so it will pop up in someones eyes if this is ever called, better that the current one

Comment on lines 57 to 74
issues.push({
error: ErrorEnum.TIER_LIMIT_EXCEEDED,
message:
`The maximum delay allowed is ${BUSINESS_TIER_MAX_DELAY_DAYS} days.` +
'Please contact our support team to discuss extending this limit for your use case.',
});
}
}

if (tier === ApiServiceLevelEnum.FREE) {
if (deferDurationMs > MAX_DELAY_FREE_TIER) {
issues.push({
error: ErrorEnum.TIER_LIMIT_EXCEEDED,
message:
`The maximum delay allowed is ${FREE_TIER_MAX_DELAY_DAYS} days.` +
'Please contact our support team to discuss extending this limit for your use case.',
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make something more structured here, the issue passed: true/ false and some metadata to build the msg, I wouldn't contain the msgs here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that use the if statments to define the max values, than validate against the comuted value and return the pass / fail, max value , max unit would be a cleaner approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure i am following your thought here, can you elaborate?

@@ -2,7 +2,9 @@ export enum StepContentIssueEnum {
MISSING_VARIABLE_IN_PAYLOAD = 'MISSING_VARIABLE_IN_PAYLOAD',
ILLEGAL_VARIABLE_IN_CONTROL_VALUE = 'ILLEGAL_VARIABLE_IN_CONTROL_VALUE',
MISSING_VALUE = 'MISSING_VALUE',
TIER_LIMIT_EXCEEDED = 'TIER_LIMIT_EXCEEDED',
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 the correct word? will it be true to other things as well? or is it specific to the world of delay & digest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i belive we could reuse it other aspects as well,

          body: [
            {
              issueType: 'TIER_LIMIT_EXCEEDED',
              message:
                'The maximum body length allowed is 280 characters. Please contact our support team to discuss extending this limit for your use case.',
            },
          ]

…-defer-duration

# Conflicts:
#	apps/api/src/app/workflows-v2/usecases/validate-content/prepare-and-validate-content/prepare-and-validate-content.usecase.ts
#	packages/shared/src/dto/workflows/step-content-issue.enum.ts
@djabarovgeorge djabarovgeorge merged commit 6f801d6 into next Nov 21, 2024
40 checks passed
@djabarovgeorge djabarovgeorge deleted the add-api-validation-on-defer-duration branch November 21, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants