APP-15551 - Update to use createIntegrationHelpers and expose entity type schemas #55
Conversation
jzolo22
changed the title
| ), | ||
| byod: SchemaType.Boolean({ | ||
| deprecated: true, | ||
| description: 'Please use BYOD instead.', |
There was a problem hiding this comment.
BYOD is part of the Device schema, correct?
There was a problem hiding this comment.
Could we add a date as to when this should be removed?
There was a problem hiding this comment.
I hesitate to do that unless we actually commit to a date everyone is good with. Kind of arbitrary but we could say Jan 1, 2025? Nice round number
src/entities.ts
Outdated
| purchaseDate: SchemaType.Optional(SchemaType.Number()), | ||
| terminationDate: SchemaType.Optional(SchemaType.Number()), | ||
| depreciation: SchemaType.Optional(SchemaType.String()), | ||
| purchaseCost: SchemaType.Optional( | ||
| SchemaType.Union([SchemaType.String(), SchemaType.Null()]), | ||
| ), | ||
| notes: SchemaType.Optional(SchemaType.Array(SchemaType.String())), | ||
| expirationDate: SchemaType.Optional(SchemaType.Number()), |
There was a problem hiding this comment.
| purchaseDate: SchemaType.Optional(SchemaType.Number()), | |
| terminationDate: SchemaType.Optional(SchemaType.Number()), | |
| depreciation: SchemaType.Optional(SchemaType.String()), | |
| purchaseCost: SchemaType.Optional( | |
| SchemaType.Union([SchemaType.String(), SchemaType.Null()]), | |
| ), | |
| notes: SchemaType.Optional(SchemaType.Array(SchemaType.String())), | |
| expirationDate: SchemaType.Optional(SchemaType.Number()), | |
| purchasedOn: SchemaType.Optional(SchemaType.Number()), | |
| terminatedOn: SchemaType.Optional(SchemaType.Number()), | |
| depreciation: SchemaType.Optional(SchemaType.String()), | |
| purchaseCost: SchemaType.Optional( | |
| SchemaType.Union([SchemaType.String(), SchemaType.Null()]), | |
| ), | |
| notes: SchemaType.Optional(SchemaType.Array(SchemaType.String())), | |
| expiresOn: SchemaType.Optional(SchemaType.Number()), |
Did we approve the date RFC?
Suggestions here are for perhaps a future iteration since these changes are breaking. Or we could deprecate and add? Thoughts?
And I just grabbed a few dates, but I imagine there are others in the PR?
There was a problem hiding this comment.
Or we could deprecate and add
I'm definitely in favor of this approach
There was a problem hiding this comment.
We haven't approved any official Date changes
There was a problem hiding this comment.
| @@ -42,12 +49,16 @@ export function convertConsumable( | |||
| notes: data.notes ? [data.notes] : undefined, | |||
| createdOn: parseTimePropertyValue(data.created_at?.datetime), | |||
| updatedOn: parseTimePropertyValue(data.updated_at?.datetime), | |||
| userCanCheckout: data.user_can_checkout, | |||
| userCanCheckout: Boolean(data.user_can_checkout), | |||
There was a problem hiding this comment.
Boolean("false") => true
Do we expect user_can_checkout to be data type boolean?
There was a problem hiding this comment.
We do, this is from the docs... but maybe wrapping in Boolean isn't necessarily the best here. I feel like I've maybe seen a boolean parsing function from the SDK?
"user_can_checkout": true,
There was a problem hiding this comment.
hmm... I think this case is fine to use Boolean(), but in general we could see booleans flip their real value.
I guess I'd prefer to see "false" instead of true on the entity when the real value is false. Does that make sense? Both are bad for query-ability, but one is completely wrong.
There was a problem hiding this comment.
Yeah I definitely agree
| @@ -51,27 +50,30 @@ export function convertHardware( | |||
| model: data.model?.name ?? null, | |||
| serial: data.serial, | |||
| byod: data.byod, | |||
| cost: Number(data.purchase_cost), | |||
| BYOD: data.byod, | |||
| cost: Number(data.purchase_cost) || undefined, | |||
There was a problem hiding this comment.
nit: future state we should make available more parsing functions.
There was a problem hiding this comment.
And then those parsing functions behavior could be describe on the docs.
So in the scenario we get $4.23 as the purchase cost, the parse function handles it and returns the number 4.34 and the docs say, we convert this to a number and remove in currency characters....
There probably is a better example...
| ...(shortLoginId && { shortLoginId }), | ||
| email: data.email, | ||
| emailDomain: emailDomain ? [emailDomain] : undefined, | ||
| shortLoginId: shortLoginId, |
There was a problem hiding this comment.
This is a change in behavior. Intentional?
There was a problem hiding this comment.
null is left on the entity whereas undefined is removed. It's minor but just wanted to confirm.
There was a problem hiding this comment.
Good callout! I wanted to make the value assignment more explicit and less conditional if that makes sense
|
@jzolo22, have we verified that the |
jzolo22
added
minor
|
🚀 PR was released in |
j1-internal-automation
added
the
released
Changes of note:
locationtosnipeit_location<-- is this a concern?? potentially need to call it out to DP