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

listitem <verb> commands do not handle errors that are returned by POST's to the REST API #4375

Closed
4 tasks done
martinlingstuyl opened this issue Jan 18, 2023 · 16 comments
Closed
4 tasks done
Assignees
Milestone

Comments

@martinlingstuyl
Copy link
Contributor

martinlingstuyl commented Jan 18, 2023

When adding a new listitem or updating an existing one, it is possible to update DateTime fields.

However: it's not clear what the format is for this. using the standard ISO format (yyyy-MM-ddTHH:mm:ssZ) does not work, on listitem add it throws the following error:

Error: Het item bestaat niet. Mogelijk is het door een andere gebruiker verwijderd.

Which occurs because the CLI tries to call the following URL with an actual 0 value:
https://contoso.sharepoint.com/sites/sales/_api/web/lists(guid'<someguid>')/items(0)

On listitem set it simply does not do anything.
But when using the --debug on update and scrolling through the logs, you can find the following:

"data": {
    "value": [
      {
        "ErrorCode": -2146232832,
        "ErrorMessage": "Voer een datum en tijd in als volgt: 23-2-2012 14:25",
        "FieldName": "SomeDateTimeFieldInternalName",
        "FieldValue": "2023-01-18",
        "HasException": true,
        "ItemId": 3
      }
    ]
  }

(Voer een datum en tijd in als volgt = Enter a date and time as follows)

So when executing a listitem add or listitem set with this new information, formatting the date like this: 23-2-2012 14:25 / d-M-yyyy HH:mm it works as expected.

Problem & Solution

The problem is that the responses of the AddValidateUpdateItemUsingPath and ValidateUpdateListItem requests are not evaluated properly. If errors are given in the request calls, the command should throw them and show them to the user.

Also there is a date format that will work in all scenario's: yyyy-HH-mm HH:mm:ss. We should update the documentation and explain how to use datetime fields. (Either in the site locale or this specific format.) We should also explain that the timezone of the site will be used.

To fix this

  • spo listitem add : The errors given by SharePoint in the AddValidateUpdateItemUsingPath request should be returned to the user.
  • spo listitem set : The errors given by SharePoint in the ValidateUpdateListItem request should be returned to the user.
  • spo listitem batch add : The errors given by SharePoint in the AddValidateUpdateItemUsingPath batch-request should be returned to the user. The batching should be broken off when an error occurs. (failfast)
  • The documentation of the commands should explain to use the date time format of the site locale or yyyy-HH-mm HH:mm:ss.
@martinlingstuyl martinlingstuyl changed the title Update listitem <verb> documentation with information how to update DateTime columns Update listitem <verb> documentation with information how to update DateTime columns Jan 18, 2023
@nicodecleyre
Copy link
Contributor

Good catch! What if we add a function that converts the given date from the input to the right format that the api expects?

@martinlingstuyl
Copy link
Contributor Author

Convert from yyyy-MM-ddTHH:mm:ssZ to d-M-yyyy HH:mm you mean? That could indeed be something we could add as well, to make the commands easier to use.

@milanholemans
Copy link
Contributor

If we do that, that would be a breaking change tho.

@waldekmastykarz
Copy link
Member

Before we commit to this change: are we sure that the d-M-yyyy HH:mm format is universal and doesn't happen to depend on the locale of the parent site?

If we do that, that would be a breaking change tho.

@milanholemans since the current functionality isn't working, I'd say it's a bug that we need to fix.

@martinlingstuyl
Copy link
Contributor Author

Good point @waldekmastykarz! This calls for some research! DateTime fields are annoying 😀

@milanholemans
Copy link
Contributor

@milanholemans since the current functionality isn't working, I'd say it's a bug that we need to fix.

Martin stated in his post that it is working in this weird format.

Original message:
So when executing a listitem add or listitem set with this new information, formatting the date like this: 23-2-2012 14:25 / d-M-yyyy HH:mm it works as expected.

@waldekmastykarz
Copy link
Member

Got it, so it's not that it's not working but it's unclear which date format to use. I suggest we investigate this across sites with different locales. If the conclusion is, that the provided date must be in the format corresponding to the locale we should add that to our docs. Additionally, we could see if there's a way for us to accept input date as ISO (so users can specify either an ISO or a locale-based date) and then convert it ourselves to the right locale just to make it easier for users.

@martinlingstuyl
Copy link
Contributor Author

martinlingstuyl commented Feb 23, 2023

OK @waldekmastykarz, research confirmed that you need to use the locale of the site you're connecting to...

I'm for treating this as a bug. The --debug option reveals that the command is actually retrieving an error message under water. We should return those errors to the user:

field values returned:
[
  {
    ErrorCode: 0,
    ErrorMessage: null,
    FieldName: 'Title',
    FieldValue: 'test123',
    HasException: false,
    ItemId: 0
  },
  {
    ErrorCode: -2146232832,
    ErrorMessage: 'Enter a date and time like this: 2/23/2012 2:25 PM',
    FieldName: 'TestDate',
    FieldValue: '15-12-2023',
    HasException: true,
    ItemId: 0
  },
  {
    ErrorCode: 0,
    ErrorMessage: null,
    FieldName: 'Id',
    FieldValue: '0',
    HasException: false,
    ItemId: 0
  }
]

Checking out out if we can use ISO dates as well might be a separate issue.

@martinlingstuyl
Copy link
Contributor Author

martinlingstuyl commented Feb 24, 2023

Something else I read somewhere and tried out. If you use 2023-02-02 01:00:02 as a format, it works on all locales. I tried it on English and Dutch. It works! A note should be added though, that the timezone of the site will be used, so ISO is only an option if we would query the site first to see what the timezone is. Not sure if that's worth it.

@waldekmastykarz
Copy link
Member

A note should be added though, that the timezone of the site will be used, so ISO is only an option if we would query the site first to see what the timezone is. Not sure if that's worth it.

This would quickly get complicated, because we'd need to get all columns specified in the command and their value types, the locale, do the validation and only then run the actually command logic. If the error we get from the API while using an invalid value is clear enough, then we don't need to perform separate validation upfront: either way running the command will fail and user will need to update the args and re-run it, so we don't really improve the user experience and just slow the command's execution down.

@martinlingstuyl
Copy link
Contributor Author

Agreed on that @waldekmastykarz, I think we'd just need the timezone of the site. But still: that's an extra call, AND you'd need to convert any ISO values to date format in that timezone. We'd probably need an extra library like momentjs for that. Let's stick with KISS.

The only downside I see of my idea is that the error message points the user to use the locale specific format, while we of the CLI folk would maybe rather want to point to the central format (2023-02-02 01:00:02) to be consistent.

@waldekmastykarz
Copy link
Member

Using momentjs for this conversion is an overkill. If we're to go down that path, I'd suggest looking for alternatives that aren't as heavy. We should also look at the Intl namespace in JS and see what's its availability around the current Node versions.

The only downside I see of my idea is that the error message points the user to use the locale specific format, while we of the CLI folk would maybe rather want to point to the central format (2023-02-02 01:00:02) to be consistent.

Ideally, I'd say we should point to a format that's human-readable for the user. Even if it means that everyone will see something different, if it makes it easier for our users, we should choose that option.

@martinlingstuyl
Copy link
Contributor Author

Even if it means that everyone will see something different, if it makes it easier for our users, we should choose that option.

What would also make it easy, is that it offers a consistent experience. If a user may see different error messages on different calls because he happens to be talking to sites of different locales, that is a less clear experience according to me.

@martinlingstuyl
Copy link
Contributor Author

martinlingstuyl commented Mar 3, 2023

By the way, I remember the last time I worked with DateTime and SharePoint, I had to use a SharePoint api endpoint to reliably convert from the localTime of a site to utc.

Something like this:
_api/web/RegionalSettings/TimeZone/localTimeToUTC(@date)?@date=

There's also a function that does it the other way around: UTCToLocalTime

So it converts any utc datetime to a local time in the zone of the site.
That would make it easier: just use SharePoint and keep JS out of it.

@waldekmastykarz
Copy link
Member

What would also make it easy, is that it offers a consistent experience. If a user may see different error messages on different calls because he happens to be talking to sites of different locales, that is a less clear experience according to me.

I guess it's all relative: for one, I don't know if it's typical to have sites in different locales in one tenant. Would users be surprised that suddenly there's a site using a different locale or would different locales belong to a separate part of the tenant and the locale's boundaries are clear.

As long as we're predictable and communicate clearly, we can go either way.

@martinlingstuyl
Copy link
Contributor Author

Just verified:
When querying this endpoint using an ISO formatted UTC datetime value, we get a nice formatted local time returned:

image

But still, say we check whether an option contains an ISO formatted datetime, we cannot be sure that the target field is actually a date time field. It could be just a text field, and the user wants to write the literal ISO value to that field.

So we cannot just implement this request on ISO formatted option values.

We could of course add functionality to the CLI to allow users to help the cli to force the target value, something like: "datetime:2023-03-01T10:00:00Z" for datetimes, but I doubt whether that is a good route.

Let's stick to the current route for now and only allow the locale-format and default format.

What do you say @waldekmastykarz?

@martinlingstuyl martinlingstuyl changed the title Update listitem <verb> documentation with information how to update DateTime columns listitem <verb> commands do not handle errors that are returned by POST's to the REST API Mar 6, 2023
martinlingstuyl added a commit to martinlingstuyl/cli-microsoft365 that referenced this issue Mar 6, 2023
martinlingstuyl added a commit to martinlingstuyl/cli-microsoft365 that referenced this issue Mar 18, 2023
martinlingstuyl added a commit to martinlingstuyl/cli-microsoft365 that referenced this issue Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment