feat: refactor template preference logic #3596
Conversation
There was a problem hiding this comment.
removed the deep eql of all the preferences now it checks the preference without the overrides, not sure if to add the override check as well.
There was a problem hiding this comment.
create small unit tests for the main functions of the usecase.
| } | ||
| } | ||
|
|
||
| export function determineOverrides( |
There was a problem hiding this comment.
the main logic of the template preferences. i centralized the logic to cannon for loop so it will be easier (i hope) to understand.
There was a problem hiding this comment.
for me it was hard to read, but I think that if you will split it to smaller functions with a good naming and will do the other suggestions it will be much better
There was a problem hiding this comment.
Yeah, I was afraid it became more complicated + the extra override source data added more complexity to it.
At some point, I was not sure if splitting this one into more functions will make it more readable or not, I will try to split it a bit more.
| export function determineOverrides( | ||
| preferenceSources: Record<'template' | 'subscriber', IPreferenceChannels> | ||
| ) { | ||
| const priorityOrder = ['template', 'subscriber']; |
There was a problem hiding this comment.
first, we override by template preference and then by the subscriber as the subscriber is on higher priority.
| it('should filter not active channels and sources', async function () { | ||
| const response = await getSubscriberPreference(session.subscriberToken); | ||
|
|
||
| const data = response.data.data[0]; | ||
|
|
||
| expect(Object.keys(data.preference.channels).length).to.equal(2); | ||
| expect(data.preference.overrides.length).to.equal(2); |
There was a problem hiding this comment.
can't we combine this one with should fetch a default user preference ?
There was a problem hiding this comment.
We could but I wanted a separate test that points to the filtering of the active channels.
...lication-generic/src/usecases/get-subscriber-preference/get-subscriber-preference.usecase.ts
Show resolved
Hide resolved
...ecases/get-subscriber-template-preference/get-subscriber-template-preference.usecase.spec.ts
Outdated
Show resolved
Hide resolved
| ).toEqual('subscriber'); | ||
| }); | ||
|
|
||
| it('should get preference from template when subscriber preference are empty', async function () { |
There was a problem hiding this comment.
we're missing the test when the template preferences are empty, shouldn't we have one?
There was a problem hiding this comment.
We can add one but I did not, because this state should not exist.
| { | ||
| email: true, | ||
| sms: true, | ||
| in_app: true, | ||
| chat: true, | ||
| push: true, | ||
| }, |
There was a problem hiding this comment.
maybe a better array of fields?
There was a problem hiding this comment.
Do you think it will be better? I am it as an object so it will be the same as the IPreferenceChannels type.
...rc/usecases/get-subscriber-template-preference/get-subscriber-template-preference.usecase.ts
Outdated
Show resolved
Hide resolved
...rc/usecases/get-subscriber-template-preference/get-subscriber-template-preference.usecase.ts
Outdated
Show resolved
Hide resolved
...rc/usecases/get-subscriber-template-preference/get-subscriber-template-preference.usecase.ts
Outdated
Show resolved
Hide resolved
...rc/usecases/get-subscriber-template-preference/get-subscriber-template-preference.usecase.ts
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| export function determineOverrides( |
There was a problem hiding this comment.
for me it was hard to read, but I think that if you will split it to smaller functions with a good naming and will do the other suggestions it will be much better
|
Hi @djabarovgeorge |
Not yet, basically, the logic remains the same, the only things that change are the following:
|
|
hey @djabarovgeorge, I've added my two cents to the code of this PR, hope it's ok for you :) just removed unnecessary functions and moved code around ;) |
| @@ -99,7 +113,9 @@ export class GetSubscriberTemplatePreference { | |||
| (step) => step.active === true | |||
| ); | |||
|
|
|||
| if (activeSteps.some((step) => !step.template)) { | |||
| const stepMissingTemplate = activeSteps.some((step) => !step.template); | |||
There was a problem hiding this comment.
I'm wondering do you know how it's possible that the step doesn't have a template?
There was a problem hiding this comment.
Yes, one of this usecase clients has a lean template that does not contain the step template. (get-subscriber-preference.usecase)
Shame you needed to, but totally fine by me because in the end, I was not sure if it is more readable or not. and your assistance really helped on getting fresh 👀 on this one 🙏 |
What change does this PR introduce?
Refactor the template preference usecase.
Add the source to the preference (template/subscriber)
Why was this change needed?
The reason for the refactoring is that the initial logic was not so intuitive, hopefully, the current one is more self-explanatory 😅.
Added the source of the override so we could print it to the execution details so the user could see why the step was filtered.
Other information (Screenshots)