Skip to content

Consolidate spo site set and spo site classic set commands. Closes #1495#3450

Closed
martinlingstuyl wants to merge 1 commit intopnp:mainfrom
martinlingstuyl:consolidateSPSiteSet
Closed

Consolidate spo site set and spo site classic set commands. Closes #1495#3450
martinlingstuyl wants to merge 1 commit intopnp:mainfrom
martinlingstuyl:consolidateSPSiteSet

Conversation

@martinlingstuyl
Copy link
Copy Markdown
Contributor

@martinlingstuyl martinlingstuyl commented Jun 29, 2022 •
edited
Loading

Closes #1495

Consolidates spo site set and spo site classic set commands.

Remarks

Because of fallback issues, The code files and docs of spo site classic set have not been removed. It has not been added as an alias to spo site set and it can still be used. All the functionality has been copied over to spo site set though. And a deprecation notice has been added to the docs.

In the next major release, we can remove the code files for spo site classic set.

Also: I've refactored the code so that: a) less calls to SharePoint are executed and b) several options are now executed against all sites, group connected or not. For example: noScriptSite and lockState, these where only used in site classic set.

@martinlingstuyl martinlingstuyl force-pushed the consolidateSPSiteSet branch 3 times, most recently from cec6983 to 5144822 Compare June 30, 2022 06:01
Adam-it
Adam-it reviewed Jul 5, 2022
View reviewed changes
Comment thread docs/docs/cmd/spo/site/site-set.md Outdated
@Adam-it Adam-it self-assigned this Jul 8, 2022
@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Jul 8, 2022

thanks @martinlingstuyl for your awesome work. Will review it shortly 😋

Adam-it
Adam-it requested changes Jul 8, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@martinlingstuyl thanks for this awesome contribution 👍you rock 🤩🤩
I tested it locally, changed a couple of site properties, lock state, applied a site design.. works really good 💪
I noticed a couple of details we might correct along the way. Please do consider them 🙏
Let me know if you will need any help 👍

Comment thread src/m365/spo/commands/site/site-set.ts Outdated
Comment thread src/m365/spo/commands/site/site-set.ts Outdated
Comment thread src/m365/spo/commands/site/site-set.ts
Comment thread src/m365/spo/commands/site/site-set.ts
Comment thread src/m365/spo/commands/site/site-set.ts
Comment thread docs/docs/cmd/spo/site/site-set.md Outdated
Comment thread src/m365/spo/commands/site/site-set.ts Outdated
@Adam-it Adam-it removed their assignment Jul 8, 2022
@Adam-it Adam-it marked this pull request as draft July 8, 2022 21:46
@Adam-it Adam-it self-assigned this Jul 17, 2022
@martinlingstuyl martinlingstuyl force-pushed the consolidateSPSiteSet branch 2 times, most recently from e6a8762 to 450e068 Compare July 18, 2022 19:25
@martinlingstuyl
Copy link
Copy Markdown
Contributor Author

martinlingstuyl commented Jul 18, 2022

good review @Adam-it 🤙

@martinlingstuyl martinlingstuyl marked this pull request as ready for review July 18, 2022 19:35
@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Jul 19, 2022

@martinlingstuyl thanks for considering my comments and adding the improvements 👍 you rock 🤩.
I noticed some test are failing (also on the gh action flow)

spo site set
❌  updates site title. wait for completion (verbose):
❌  updates title of the specified groupified site:
❌ updates all properties. wait for completion, immediately complete:
❌  correctly handles error when updating title of the specified site:
❌  escapes XML in the request:

do you think it's possible for you to also have a check on this one as well 🙏? I will mark the PR as draft for now until we fix up the tests 😉. Let me know if you would need any help I would love to do so 👍

@Adam-it Adam-it marked this pull request as draft July 19, 2022 06:00
@martinlingstuyl
Copy link
Copy Markdown
Contributor Author

martinlingstuyl commented Jul 19, 2022

I apparently broke the tests with my last fix. Missed two ! symbols. Never assume the code will work. 😂

@martinlingstuyl martinlingstuyl marked this pull request as ready for review July 19, 2022 07:25
Adam-it
Adam-it approved these changes Jul 19, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

✅👍🤩

@Adam-it Adam-it removed their assignment Jul 19, 2022
@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Jul 19, 2022

you rock 🤩merged manually 👍

@Adam-it Adam-it closed this Jul 19, 2022
@martinlingstuyl martinlingstuyl deleted the consolidateSPSiteSet branch July 20, 2022 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Adam-it Adam-it Adam-it approved these changes

Assignees

No one assigned

Labels

pr-merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consolidate spo site set and spo site classic set commands

2 participants

@martinlingstuyl @Adam-it