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

Adds 'spe containertype get' command. Closes #5991 #6228

Closed
wants to merge 8 commits into from

Conversation

reshmee011
Copy link
Contributor

Linked Issue

Closes #5991

New Command = Five Files

  • src/m365/spe/commands.ts
  • src/m365/spe/commands/containertype/containertype-get.ts
  • src/m365/spe/commands/containertype/containertype-get.spec.ts
  • docs/src/config/sidebars.js
  • docs/cmd/spe/containertype/containertype-get.mdx

@milanholemans
Copy link
Contributor

Thank you @reshmee011, we'll try to review it ASAP!

@Adam-it Adam-it self-assigned this Oct 15, 2024
Copy link
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.

@reshmee011 awesome work so far 👏👏👏
I ma really impressed how many contributions you create to different PnP repositories and truly honored CLI for M365 is on your list 😍.
I added some small comments. Please do give them a double check before we proceed 🙏🙏

docs/src/config/sidebars.ts Outdated Show resolved Hide resolved
src/m365/spe/commands.ts Outdated Show resolved Hide resolved
src/m365/spe/commands/containertype/containertype-get.ts Outdated Show resolved Hide resolved
src/m365/spe/commands/containertype/containertype-get.ts Outdated Show resolved Hide resolved
src/m365/spe/commands/containertype/containertype-get.ts Outdated Show resolved Hide resolved
@Adam-it Adam-it marked this pull request as draft October 16, 2024 21:05
@reshmee011
Copy link
Contributor Author

@Adam-it : Thanks for the kind words. Contributing to PnP repos is the only way I am doing any coding :). I am still learning. Your contributions are amazing. I will look at the comments and remediate the code.

@reshmee011 reshmee011 marked this pull request as ready for review October 17, 2024 14:18
Copy link
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.

Awesome work 👏👏👏👏
You Rock 🤩

Comment on lines +20 to +22
class SpeContainertypeGetCommand extends SpoCommand {

public get name(): string {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class SpeContainertypeGetCommand extends SpoCommand {
public get name(): string {
class SpeContainertypeGetCommand extends SpoCommand {
public get name(): string {

@Adam-it
Copy link
Member

Adam-it commented Oct 23, 2024

Ready to merge 🚀 Added small styling comment I will resolve when merging

Comment on lines +131 to +152
private async getContainerTypeIdByName(name: string, spoAdminUrl: string, logger: Logger): Promise<string> {
const formDigestInfo: FormDigestInfo = await spo.ensureFormDigest(spoAdminUrl, logger, undefined, this.debug);

const requestOptions: CliRequestOptions = {
url: `${spoAdminUrl}/_vti_bin/client.svc/ProcessQuery`,
headers: {
'X-RequestDigest': formDigestInfo.FormDigestValue
},
data: `<Request AddExpandoFieldTypeSuffix="true" SchemaVersion="15.0.0.0" LibraryVersion="16.0.0.0" ApplicationName="${config.applicationName}" xmlns="http://schemas.microsoft.com/sharepoint/clientquery/2009"><Actions><ObjectPath Id="46" ObjectPathId="45" /><Method Name="GetSPOContainerTypes" Id="47" ObjectPathId="45"><Parameters><Parameter Type="Enum">1</Parameter></Parameters></Method></Actions><ObjectPaths><Constructor Id="45" TypeId="{268004ae-ef6b-4e9b-8425-127220d84719}" /></ObjectPaths></Request>`
};

const res: string = await request.post(requestOptions);
const json: ClientSvcResponse = JSON.parse(res);
const containerTypes: ContainerTypeProperties[] = json[json.length - 1];

if (!containerTypes.find(c => c.DisplayName === name)) {
throw new Error(`Container type with name '${name}' not found`);
}
const match = containerTypes.find(c => c.DisplayName === name)!.ContainerTypeId.match(/\/Guid\(([^)]+)\)\//);
return match![1];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@reshmee011 I just saw an awesome improvement when reviewing other PR's. I noticed in @nanddeepn in his PR he refactored to get all container types to an util
https://github.com/pnp/cli-microsoft365/pull/6412/files?diff=unified&w=0#diff-40e186d5ac89ee5a7afb93d0d6acb72b5614f059fef9e584dc61497b90d72e0f

we could reuse the same util here instead of having the same logic in command.
No action is required from your side I just wanted to let you know that I will put merging this 'on hold' until I merge #6412 and then I will refactor this PR to reuse @nanddeepn util and approach.

Thank you both for awesome contributions and great team work 👏👏👏👏

@Adam-it
Copy link
Member

Adam-it commented Nov 5, 2024

Merged manually. Awesome work 👏. You Rock 🤩

@Adam-it Adam-it closed this Nov 5, 2024
@reshmee011 reshmee011 deleted the getContainerType branch November 7, 2024 06:56
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.

New command: m365 spe containertype get
3 participants