Bug-fix: spo site remove#5955
Closed
MathijsVerbeeck wants to merge 1 commit intopnp:mainfrom
Closed
Conversation
Contributor
|
Thanks @MathijsVerbeeck, we'll have a look at it ASAP! |
Contributor
Author
|
Please keep in mind to create a new issue to remove the wait option in v8. |
Contributor
Thanks for the reminder. |
milanholemans
requested changes
Apr 21, 2024
Contributor
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Nice work @MathijsVerbeeck, command looks a whole lot better already. Could you have a look at the comments I made before we continue?
Contributor
Author
|
Thanks for the thorough review. Could you have another look? |
MathijsVerbeeck
force-pushed
the
bug-fix-spo-site-remove
branch
from
75cf2aa to
594a2a8
Compare
milanholemans
requested changes
Apr 23, 2024
Contributor
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
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?
milanholemans
requested changes
May 4, 2024
Contributor
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Did another review. Command is working almost perfectly, few more changes are needed.
MathijsVerbeeck
force-pushed
the
bug-fix-spo-site-remove
branch
from
1ad309a to
86c439a
Compare
milanholemans
force-pushed
the
bug-fix-spo-site-remove
branch
from
86c439a to
62ff25b
Compare
milanholemans
approved these changes
May 6, 2024
Contributor
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Great work, made a few last changes while merging.
| if (options.skipRecycleBin) { | ||
| 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.`); |
Contributor
There was a problem hiding this comment.
Since we added another verbose log in the function itself, we can remove this one.
| let amountOfPolls = 0; | ||
|
|
||
| await this.deleteOrphanedSite(logger, args.options.url); | ||
| while (!isGroupInRecycleBin && amountOfPolls < 23) { |
Contributor
There was a problem hiding this comment.
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.`; |
Contributor
There was a problem hiding this comment.
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 () => { |
Contributor
There was a problem hiding this comment.
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 () => { |
Contributor
There was a problem hiding this comment.
Let's also check for the remove request.
| ]); | ||
| } | ||
| } | ||
| it('fails validation if siteUrl is a valid url', async () => { |
| assert.notStrictEqual(actual, true); | ||
| }); | ||
|
|
||
| it('fails validation if both fromRecycleBin and skipRecycleBin is specified is a valid url', async () => { |
Contributor
There was a problem hiding this comment.
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 { |
Contributor
There was a problem hiding this comment.
Let's rename to removeTrailingSlashes since it removes multiple slashes.
Contributor
|
Merged manually, thanks for fixing this issue! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5218