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

Fix commands where we're unable to set options to empty values because of wrong conditions #4331

Open
13 tasks
martinlingstuyl opened this issue Jan 11, 2023 · 3 comments

Comments

@martinlingstuyl
Copy link
Contributor

For certain commands, setting an option to an empty value will not take care of resetting a value in MS365. This is caused by badly written conditions.

take the following example from spo list set

private mapRequestBody(options: Options): any {
    const requestBody: any = {};

    if (options.newTitle) {
      requestBody.Title = options.newTitle;
    }
    
    if (options.description) {
      requestBody.Description = options.description;
    }

   //...
}

This function builds a request object to post to SharePoint. The problem is that using an empty string for description will cause the runtime to skip the if-statement.

That's because an empty string is considered a falsy value in JavaScript, when used in a condition.
So if options.description is an empty string, if (options.description) { } will be evaluated to false.

The right check in this case should be an explicit undefined check:

if (options.description !== undefined) { }

or

if (typeof options.description !== "undefined") { }

Originally posted by @martinlingstuyl in #3718 (comment)

Commands

This impacts primarily set commands, where a user would set something to empty. The situation occurs as far as I can see in the following commands (but there might be more):

  • spo list set
  • spo customaction set
  • teams channel set
  • teams team set
  • todo task set
  • aad o365group set
  • aad user set
  • graph schemaextension set
  • spo web set
  • spo site set
  • spo sitedesign set
  • spo sitescript set
  • planner plan set
@Jwaegebaert
Copy link
Contributor

We should be careful with this change and reflect on each value if they are allowed to pass an empty string. Some values won't allow this and will cause a schema validation error to be thrown. e.g. for the command planner task set, when you want to change the title to an empty string you'll get the following.

afbeelding

This is a healthy error to have in the API but the error thrown could be confusing for the users. To counter this, we could also include additional validation checks to validate these occurences. That way we can implement the change to the methods mapRequestBody that you suggested and be sure that the proper validation error is thrown for the non-empty values.

@martinlingstuyl
Copy link
Contributor Author

Absolutely agreed on that @Jwaegebaert, maybe we should pick this up command by command. I'll change this to an epic.

@martinlingstuyl martinlingstuyl changed the title Bug report: unable to set options to empty Unable to set options to empty values because of wrong conditions. Jan 11, 2023
@martinlingstuyl martinlingstuyl changed the title Unable to set options to empty values because of wrong conditions. Fix commands where we're unable to set options to empty values because of wrong conditions Jan 11, 2023
@waldekmastykarz
Copy link
Member

Absolutely agreed on that @Jwaegebaert, maybe we should pick this up command by command. I'll change this to an epic.

Additionally, let's create a separate issue for each command to modify and let's spec which options allow empty values and should be changed.

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

No branches or pull requests

3 participants