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

Date handling doesn't work very well. #2

Open
spadusa opened this issue Jan 3, 2017 · 6 comments
Open

Date handling doesn't work very well. #2

spadusa opened this issue Jan 3, 2017 · 6 comments
Labels

Comments

@spadusa
Copy link

spadusa commented Jan 3, 2017

If I try to create a new Payment on an Invoice, I might do something like this:

let newPayment: Payment = {
                            type: 'Payment',
                            invoice: {
                                id: '123123'
                            },
                            paymentDate: '2016-12-05',
                            appliedBy: 'SomeSource',
                            amount: '400'
                        };

connectwise.InvoicePaymentsApi.financeInvoicesIdPaymentsPost({id: 123123, payment: newPayment})

And it would submit a request with the date. And it would fail on the CW side with an Unsupported format applied to paymentDate. Further, Typescript throws a Type warning on the paymentDate line because it's defined as a Date type and not a string type.

if I change the paymentDate line to
paymentDate: new Date()

Now Typescript doesn't complain about it, but CW still does as it gets sent out as paymentDate: "2016-12-30T05:00:00.000Z"

I'd recommend either handling all dates in a consistent manner, or if time is a constraint, making all date fields a union type of string|Date and let the individual dev pass it in the correct format without TypeScript yelling about it.

Ultimately I'm using MomentJS for all my datetime stuff, so I'm just going to ignore the Typescript complaining for now. However, I'm sure you know that it's a real pain for the IDE to complain about this while Typescript still compiles and works fine.

@sowderca sowderca self-assigned this Jan 3, 2017
@mattheyan
Copy link

Hi @spadusa,

Date/time is always a problem area, isn't it ;-)

Can you clarify what you mean by this:

Now Typescript doesn't complain about it, but CW still does as it gets sent out as paymentDate: "2016-12-30T05:00:00.000Z"

FWIW, I'm using this module in a project that creates time entries, which have two date/time properties. I'm posting a date/time string that looks like the one in your example and it works. So, I'm curious why CW would complain about it in your payment example.

By the way, this is Andreas, right?

Thanks,
Bryan

@spadusa
Copy link
Author

spadusa commented Jan 5, 2017

Yes, this is Andreas :-) Hi!

And yes, I think whoever invented the concept of time on a computer was out to damn every programmer until the end of time. I worked with PHP for almost 10 years and the empty time displays as 12/31/1969 19:00:00 (on the east coast, anyway). That date/time will forever haunt my dreams. I see legitimate instances of that date and immediately question whether it's real or an error....

Anyway...

I use IntelliJ IDEA as my IDE and it has decent TypeScript support. When I use paymentDate = new Date() the type matches the defined type of Date so TypeScript doesn't throw a warning. Everything looks like it works fine. But CW throws it out because of the millisecond part of the date that gets sent. Look carefully at the date... 2016-12-30T05:00:00.000Z has a millisecond piece that CW doesn't like. CW wants 2016-12-30T05:00:00Z

Also, FWIW, I just made the type of paymentDate Date|string and passed the format I wanted. Definitely much dirtier than correct DateTime handling. Ideally there'd be a singular datetime utility to format the datetime correctly when sent to CW. It's also possible that CW discriminates depending on which fields are being used. For instance, if your datetime has the millisecond component and CW accepts it, maybe that particular method of the API accepts that particular format and this whole thing is actually a bug with CW's API.....

It is somewhat clear that they don't expect the millisecond component based on the example date on this page, though "https://developer.connectwise.com/Manage/REST/Formatting_Requests"

@mattheyan
Copy link

Hi Andreas,

Ah, the milliseconds! I noticed that (mine doesn't include milliseconds) but didn't think it would cause a problem. Apparently a bad assumption on my part.

I'll check with Cameron about what we can do to address it.

Thanks
Bryan

@spadusa
Copy link
Author

spadusa commented Jan 5, 2017

Bryan,

If I get some time and can poke more at the code, I'll let you know and submit a PR, but I doubt it'll be any time soon.

As an aside, I noticed that your tests are excluded in the .gitconfig. Is there a reason for that? It would great to have the tests to write against if I ever do submit another PR.

Thanks,
Andreas

@hoffmanmc
Copy link

I spent days not knowing why my Opportunity wouldn't post, throwing this error when the 'expectedCloseDate' I provided matched the documentation exactly. We had to delay the integration by a day because the docs are incorrect. At the very least, you need to update the documentation to match what the API actually accepts. This thread is 4 years old!!

https://developer.connectwise.com/Products/Manage/REST#/Opportunities/postSalesOpportunities

@sowderca
Copy link
Contributor

sowderca commented Sep 1, 2021

@hoffmanmc Yeah... the module would need to be regenerated using the OpenAPI / Swagger spec that ConnectWise provides.

Unfortunately I don't have a developer account with them anymore so I can't do it myself though I do have the the script saved here that was used to generate the merged master spec. The URL's will likely need to be updated to pull from wherever they are storing the specs currently though.

I'll throw out that I do have a bit newer version of the module here if that helps.

Someone else from @vc3/automation might be able to help out more with this though.

If you do regenerate the client I'd be happy to approve a PR and publish a newer version of the module though.

Edit: An alternative would be were the specifications to be provided to me I'd be happy to release an update to the module myself.

@sowderca sowderca removed their assignment Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants