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

fix: Multi group grant page becomes public when one of groups deleted #8518

Conversation

arafubeatbox
Copy link
Contributor

@arafubeatbox arafubeatbox commented Feb 24, 2024

task

https://redmine.weseek.co.jp/issues/141311

管理画面イメージ

「公開可能なページを公開する」選択時のみ補足は表示される。
image

});

/*
* Update UserGroup
*/
test('Updated values should be reflected. (name, description, parent)', async() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここら辺の既存部分への変更は、メソッド単位でテストを describe で囲んだだけです。

const userGroupRelation12AfterRemove = await UserGroupRelation.findOne({ relatedGroup: userGroup12._id, relatedUser: userId1 });
await expect(userGroupRelation11AfterRemove).toBeNull();
await expect(userGroupRelation12AfterRemove).toBeNull();
describe('removeCompletelyByRootGroupId', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここから下が新規テストになります。

@arafubeatbox arafubeatbox changed the title Fix/141309 141311 multi group grant page becomes public when one of groups deleted fix: Multi group grant page becomes public when one of groups deleted Feb 24, 2024
@@ -1531,6 +1531,7 @@ describe('Page', () => {
// userB group remains, although options does not include it
{ item: userGroupIdPModelB, type: GroupType.userGroup },
]));
expect(normalizeGrantedGroups(page.grantedGroups).length).toBe(3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

過去テスト補強:
expect.arrayContaining は array の中身に要素群がいるかを確認するだけで、完全一致かどうかは確認していなかったため、length の assertion を追加。

@arafubeatbox arafubeatbox marked this pull request as ready for review February 25, 2024 07:12
@@ -196,14 +195,19 @@ export const UserGroupDeleteModal: FC<Props> = (props: Props) => {
</ModalBody>
<ModalFooter>
<form className="d-flex justify-content-between w-100" onSubmit={handleSubmit}>
<div className="d-flex mb-0">
<div className="d-flex mb-0 me-3 ">
Copy link
Member

Choose a reason for hiding this comment

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

なんかおかしなスペースがある

Copy link
Contributor Author

Choose a reason for hiding this comment

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

修正しました。

{renderPageActionSelector()}
{renderGroupSelector()}
</div>
<button type="submit" value="" className="btn btn-sm btn-danger text-nowrap" disabled={!validateForm()}>
<span className="material-symbols-outlined">delete_forever</span> {t('Delete')}
</button>
</form>
{actionName === 'public' && (
Copy link
Member

Choose a reason for hiding this comment

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

ついでに public, delete, transfer を const (enum 的な) に変えてください

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PageActionOnGroupDelete を定義しました。


switch (action) {
case 'public':
await Page.publicizePages(pages);
case 'public': {
Copy link
Member

Choose a reason for hiding this comment

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

const (enum) を参照して挙動を変える

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PageActionOnGroupDelete を定義しました。

},
};
});
await Page.bulkWrite(queries);
Copy link
Member

Choose a reason for hiding this comment

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

Page.publicizePages の引数を拡張して groupsToDeleteIds を渡して、その中で updateOne クエリを変える方がよさそうだなあ
そうすると名前も publicize じゃない方がいいのかもしれない

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removeGroupsToDeleteFromPages として実装し直しました。

Copy link

reg-suit bot commented Feb 27, 2024

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴


🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

@arafubeatbox arafubeatbox merged commit 5cc1d2e into dev/7.0.x Feb 28, 2024
16 of 21 checks passed
@arafubeatbox arafubeatbox deleted the fix/141309-141311-multi-group-grant-page-becomes-public-when-one-of-groups-deleted branch February 28, 2024 09:34
@github-actions github-actions bot mentioned this pull request Feb 28, 2024
@yuki-takei yuki-takei mentioned this pull request Mar 26, 2024
@github-actions github-actions bot mentioned this pull request Mar 26, 2024
@github-actions github-actions bot mentioned this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants