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

Enhances command spo list remove #6378

Closed
wants to merge 2 commits into from
Closed

Conversation

ktskumar
Copy link
Contributor

closes #6270

In this PR, i have added a recycle option to spo list remove command

@milanholemans
Copy link
Contributor

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

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

Nice start, let's do a few enhancements before we continue.

src/m365/spo/commands/list/list-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/list/list-remove.ts Show resolved Hide resolved
src/m365/spo/commands/list/list-remove.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/list/list-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/list/list-remove.spec.ts Outdated Show resolved Hide resolved
docs/docs/cmd/spo/list/list-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/list/list-remove.mdx Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft October 15, 2024 22:40
@ktskumar
Copy link
Contributor Author

I have done rebase to main and added the commit. Is it fine or do I need to resolve anything from my end. Please guide me

@milanholemans
Copy link
Contributor

@ktskumar seems like something went horribly wrong while rebasing with the latest main. Could you have a look at it, please?
When you are done, you can mark the PR as ready for review.

@ktskumar ktskumar marked this pull request as ready for review October 16, 2024 10:19
Copy link
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.

Apart from a few comments, it looks good to go!

promptIssued = true;
return Promise.resolve(false);
//return Promise.resolve(false);
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 comments

sinon.stub(request, 'post').callsFake(async (opts) => {
requests.push(opts);
if (opts.url === `${webUrl}/_api/web/lists/GetByTitle('${listTitle}')/recycle`) {
return 'Correct URL';
Copy link
Contributor

Choose a reason for hiding this comment

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

This API doesn't return any result. Let's also return nothing in our stub.

Comment on lines +161 to +165
requests.forEach(r => {
if (r.url === `${webUrl}/_api/web/lists/GetByTitle('${listTitle}')/recycle`) {
correctRequestIssued = true;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, let's use assert(postStub.calledOnce). This ensures that the request was called once and didn't throw an error.

@milanholemans
Copy link
Contributor

Merged manually, thanks!

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.

Enhancement: spo list remove add possibility to recycle
2 participants