Skip to content

Fixes 'commsite enable' issue. Closes #5154#5174

Closed
Saurabh7019 wants to merge 2 commits intopnp:mainfrom
Saurabh7019:issue-5154
Closed

Fixes 'commsite enable' issue. Closes #5154#5174
Saurabh7019 wants to merge 2 commits intopnp:mainfrom
Saurabh7019:issue-5154

Conversation

@Saurabh7019
Copy link
Copy Markdown
Contributor

@Saurabh7019 Saurabh7019 commented Jun 27, 2023 •
edited
Loading

@Saurabh7019 Saurabh7019 marked this pull request as ready for review June 27, 2023 08:57
@Jwaegebaert
Copy link
Copy Markdown
Contributor

Jwaegebaert commented Jun 27, 2023

Awesome, we'll try to review it asap

milanholemans
milanholemans reviewed Jun 27, 2023
View reviewed changes
Comment thread docs/docs/cmd/spo/site/site-commsite-enable.mdx

`-i, --designPackageId [designPackageId]`
: The ID of the site design to apply when enabling communication site features
: The ID of the site design to apply when enabling communication site features. Allowed values are `96c933ac-3698-44c7-9f4a-5fd17d71af9e` (Topic = default), `6142d2a0-63a5-4ba0-aede-d9fefca2c767` (Showcase), `f6cc5403-0d63-442e-96c0-285923709ffc` (Blank)
Copy link
Copy Markdown
Contributor

@milanholemans milanholemans Jun 27, 2023

Choose a reason for hiding this comment

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

Instead of providing the IDs, I'm wondering if we should accept the user-friendly names instead. But that would be a breaking change we have to address in v7.

@milanholemans milanholemans mentioned this pull request Jul 4, 2023
Closed
@plamber
Copy link
Copy Markdown
Contributor

plamber commented Jul 9, 2023

Hi @milanholemans and @Saurabh7019,
could you please also ensure that this command includes the output responses in the help? I am currently reviewing #5159 from @Saurabh7019. He covered all response outputs except the one for this command. It would make sense to add the change here to avoid merge conflicts.

Cheers

@Saurabh7019
Copy link
Copy Markdown
Contributor Author

Saurabh7019 commented Jul 9, 2023

Hi Patrik, This PR includes the output response. The command had an issue, and I was not able to get the output, which is why it was not included in PR #5159.

@milanholemans milanholemans self-assigned this Jul 9, 2023
milanholemans
milanholemans requested changes Jul 15, 2023
View reviewed changes
Copy link
Copy Markdown
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 job so far @Saurabh7019
Let's change a few things before we proceed.

Comment thread src/m365/spo/commands/site/site-commsite-enable.ts Outdated
Comment thread src/m365/spo/commands/site/site-commsite-enable.ts Outdated
Comment thread src/m365/spo/commands/site/site-commsite-enable.ts Outdated
Comment thread src/m365/spo/commands/site/site-commsite-enable.ts
Comment thread src/m365/spo/commands/site/site-ensure.spec.ts Outdated
Comment thread src/m365/spo/commands/site/site-groupify.spec.ts Outdated
Comment thread src/m365/spo/commands/site/site-commsite-enable.spec.ts Outdated
@milanholemans milanholemans marked this pull request as draft July 15, 2023 20:10
@Saurabh7019 Saurabh7019 marked this pull request as ready for review July 18, 2023 21:05
milanholemans
milanholemans requested changes Jul 20, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Almost there @Saurabh7019
Last changes before we merge it.

Comment thread src/m365/spo/commands/site/site-commsite-enable.ts Outdated
Comment thread src/m365/spo/commands/site/site-commsite-enable.spec.ts Outdated
@milanholemans milanholemans marked this pull request as draft July 20, 2023 21:32
@Saurabh7019 Saurabh7019 marked this pull request as ready for review July 21, 2023 04:09
milanholemans
milanholemans approved these changes Jul 21, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Nothing to add, great work squashing this bug 👏

@milanholemans
Copy link
Copy Markdown
Contributor

milanholemans commented Jul 21, 2023

Merged manually. Thank you for squashing this bug and refactoring the command to use the SP REST API!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milanholemans milanholemans milanholemans approved these changes

Assignees

@milanholemans milanholemans

Labels

pr-bugfix pr-merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug report: Error: Method "EnableCommSite" does not exist

4 participants

@Saurabh7019 @Jwaegebaert @plamber @milanholemans