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

Bug-fix: spo site remove #5955

Closed

Conversation

MathijsVerbeeck
Copy link
Contributor

Closes #5218

@milanholemans
Copy link
Contributor

Thanks @MathijsVerbeeck, we'll have a look at it ASAP!

@MathijsVerbeeck
Copy link
Contributor Author

Please keep in mind to create a new issue to remove the wait option in v8.

@milanholemans
Copy link
Contributor

Please keep in mind to create a new issue to remove the wait option in v8.

Thanks for the reminder.

@milanholemans milanholemans self-assigned this Apr 6, 2024
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Nice work @MathijsVerbeeck, command looks a whole lot better already. Could you have a look at the comments I made before we continue?

src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
docs/docs/cmd/spo/site/site-remove.mdx Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft April 21, 2024 16:25
@MathijsVerbeeck
Copy link
Contributor Author

Thanks for the thorough review. Could you have another look?

Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

The command is getting a nice shape. Unfortunately, I encountered some issues which we should definitely have a look at. Could you have a look at the comments I made before I do another test run?

src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft April 23, 2024 22:53
@MathijsVerbeeck MathijsVerbeeck marked this pull request as ready for review April 24, 2024 13:35
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Did another review. Command is working almost perfectly, few more changes are needed.

src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/site/site-remove.spec.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft May 4, 2024 20:42
@MathijsVerbeeck MathijsVerbeeck marked this pull request as ready for review May 5, 2024 19:18
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Great work, made a few last changes while merging.

if (this.verbose) {
await logger.logToStderr(`Site group doesn't exist. Searching in the Microsoft 365 deleted groups.`);
await logger.logToStderr(`Checking if group '${siteDetails.GroupId}' is already permanently deleted from recycle bin.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we added another verbose log in the function itself, we can remove this one.


await this.deleteOrphanedSite(logger, args.options.url);
while (!isGroupInRecycleBin && amountOfPolls < 23) {
Copy link
Contributor

Choose a reason for hiding this comment

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

23 looks like an arbitrary number, no?


if (options.fromRecycleBin) {
if (!siteDetails.TimeDeleted) {
throw `Site is currently not in the recycle bin. Do not specify --fromRecycleBin if you want to try to remove it from the active sites.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's sound a bit more confident :)

Suggested change
throw `Site is currently not in the recycle bin. Do not specify --fromRecycleBin if you want to try to remove it from the active sites.`;
throw `Site is currently not in the recycle bin. Remove --fromRecycleBin if you want to remove it as active site.`;


}
}
it('deletes a group site, deletes the m365 group from entra id and immediately deletes the site from the recycle bin, but skips deletion of the m365 group when it does not exist in Entra', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the title, we should check if delete was not called instead of amountOfPolls.


throw 'Invalid request';
});
it('aborts removing the site when prompt not confirmed', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also check for the remove request.

]);
}
}
it('fails validation if siteUrl is a valid url', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Invalid*

assert.notStrictEqual(actual, true);
});

it('fails validation if both fromRecycleBin and skipRecycleBin is specified is a valid url', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('fails validation if both fromRecycleBin and skipRecycleBin is specified is a valid url', async () => {
it('fails validation if both fromRecycleBin and skipRecycleBin are specified', async () => {

* @param url The URL to process.
* @returns The URL without trailing slash.
*/
removeTrailingSlash(url: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename to removeTrailingSlashes since it removes multiple slashes.

@milanholemans
Copy link
Contributor

Merged manually, thanks for fixing this issue!

@MathijsVerbeeck MathijsVerbeeck deleted the bug-fix-spo-site-remove branch May 25, 2024 10:07
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.

Bug report - 'spo site remove' - Removing sites and groupified sites does not work correctly
2 participants