-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Nv 1576 make default layout changes dependent #2711
Nv 1576 make default layout changes dependent #2711
Conversation
NV-1576 Make default layout changes dependent
When changing a layout to be default, 2 changes are created (layout se to default and prev layout to be not default) But, they are promoted separately, so could create a situation where there are 2 default layouts in prod. |
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.
Great job😍 Wrote a few questions.
@@ -93,4 +109,27 @@ export class SetDefaultLayoutUseCase { | |||
): Promise<void> { | |||
await this.layoutRepository.updateIsDefault(layoutId, environmentId, organizationId, isDefault); | |||
} | |||
|
|||
private async createDefaultChange(command: CreateDefaultLayoutChangeCommand) { | |||
const createLayoutChangeCommand = CreateDefaultLayoutChangeCommand.create({ |
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.
I think you might be creating this command twice :) here and in line 55
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.
😅 Removing them
|
||
if (!existingDefaultLayoutId) { | ||
await this.createDefaultChange( | ||
CreateDefaultLayoutChangeCommand.create({ | ||
environmentId: command.environmentId, | ||
layoutId: command.layoutId, | ||
organizationId: command.organizationId, | ||
userId: command.userId, | ||
}) | ||
); | ||
|
||
return; |
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.
From what I understand here !existingDefaultLayoutId will be true when the set default is called for the already default layout. So no change should be created here. Wdyt? Am I missing something?
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.
For the very first layout, the existingDefaultId
will be false,
This will handle that scenario
@@ -39,10 +39,10 @@ export class CreateLayoutUseCase { | |||
userId: dto._creatorId, | |||
}); | |||
await this.setDefaultLayout.execute(setDefaultLayoutCommand); | |||
} else { | |||
await this.createChange(command, dto._id); |
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.
I wonder what would happen if there is an update/create of layout(name,content) + setting to default. Would it create 2 changes? One for default layout change and one for layout change? If so, are we sure they are also dependent on promote?
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 that case, It will create a single(1
) change under DefaultLayout
,
This way it maintains the dependency between other layout changes too.
Looks good! Then my only concern is the message in the changes table. It shows it only as one row with Default Layout Change and just one of the layouts (I think the one that used to be default) with all the fields changes, not only the default change. Also, it will include changes made in between to another template - I just worry it will be a bit confusing for users to understand what they are promoting. |
apps/api/src/app/change/usecases/get-changes/get-changes.usecase.ts
Outdated
Show resolved
Hide resolved
...c/app/change/usecases/promote-change-to-environment/promote-change-to-environment.usecase.ts
Outdated
Show resolved
Hide resolved
{type === ChangeEntityTypeEnum.DEFAULT_LAYOUT && ( | ||
<Text color={colorScheme === 'dark' ? colors.B40 : colors.B70}>Default Layout Change</Text> | ||
)} |
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.
similar to the line above
dab47c1
to
08a5b73
Compare
bf78ec5
to
2cddfa4
Compare
What change does this PR introduce?
DefaultLayout
Why was this change needed?
Other information (Screenshots)