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 command pp website weblink list #6449

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

ktskumar
Copy link
Contributor

Closes #6269

Added new command pp website weblink list

@Jwaegebaert Jwaegebaert changed the title Issue 6269 Adds command pp webiste weblink list Oct 25, 2024
@Jwaegebaert Jwaegebaert changed the title Adds command pp webiste weblink list Adds command pp website weblink list Oct 25, 2024
@Adam-it Adam-it added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 29, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 29, 2024

@ktskumar I added the hacktoberfest-accepted label to this PR which means that this PR will count as done for the Hacktoberfest event. So if you participate in this event it will get you unblocked and it allows us to merge this PR later when we catch up 👍
Thanks for your support and awesome contribution 👏 You Rock 🤩

@martinlingstuyl martinlingstuyl self-assigned this Nov 19, 2024
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @ktskumar,

Great work! I do have some comments, could you look at those and get back to me? Let me know if you have any questions!

import PowerPlatformCommand from '../../../base/PowerPlatformCommand.js';
import commands from '../../commands.js';
import { PpWebSiteOptions } from '../Website.js';

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 remove superfluous white lines

if (args.options.websiteId) {
return args.options.websiteId;
}
const options: PpWebSiteOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a white line after an if-scope.
And also add a verbose logging line saying it will retrieve the website Id by name

responseType: 'json'
};

if (options.name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this options.name doing here? I think you should not need to check if the website name has a value. Its an option set with websiteId, and we've already ascertained in the validation that a value should be there in one of the options.

return args.options.websiteId;
}
const options: PpWebSiteOptions = {
environmentName: args.options.environmentName,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this options object is not really in use. So we can remove it. Further down you're referencing options.name, but it's more logical there to use args.options.websiteName

if (result.value.length === 0) {
throw `The specified website '${args.options.websiteName}' does not exist.`;
}
return result.value[0].powerpagesiteid;
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 add a white line after an if-scope

}

private async getwebsitelinksets(dynamicsApiUrl: string, websiteId: string): Promise<any> {

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 remove this linebreak

}
}

private async getwebsitelinksets(dynamicsApiUrl: string, websiteId: string): Promise<any> {
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 move the two private functions to below the public commandAction function.

Also, I'd suggest a rename on this function:

getwebsitelinksets > getWebLinkSets

return options;
}

private async getWebSiteId(dynamicsApiUrl: string, args: CommandArgs): Promise<any> {
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
private async getWebSiteId(dynamicsApiUrl: string, args: CommandArgs): Promise<any> {
private async getWebsiteId(dynamicsApiUrl: string, args: CommandArgs): Promise<any> {

responseType: 'json'
};

if (websiteId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if-statement is superfluous I believe, as the websiteId parameter is not nullable.

try {
const dynamicsApiUrl = await powerPlatform.getDynamicsInstanceApiUrl(args.options.environmentName, args.options.asAdmin);
const websiteId = await this.getWebSiteId(dynamicsApiUrl, args);
const weblinksets = await this.getwebsitelinksets(dynamicsApiUrl, websiteId);
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
const weblinksets = await this.getwebsitelinksets(dynamicsApiUrl, websiteId);
const weblinkSets = await this.getwebsitelinksets(dynamicsApiUrl, websiteId);

@martinlingstuyl martinlingstuyl marked this pull request as draft November 19, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accept for hacktoberfest, will merge later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: m365 pp website weblink list
4 participants