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

Minor updates #2

Merged
merged 1 commit into from
May 13, 2021
Merged

Minor updates #2

merged 1 commit into from
May 13, 2021

Conversation

yakkomajuri
Copy link
Contributor

Changes

Hey! Thank you so much for this! It's super cool!

I've made a few changes to incorporate some best practices. Feel free to push back / ask more about it.

processEventBatch

We're deprecating this with the new release that's coming out today, so I moved this plugin to a new approach using onEvent. onEvent is used for plugins that need to read the event but don't modify it, as is the case here. Docs for this are coming out today.

statusOk

The current checking mechanism would only match a 200, in which case it could just check if the status is equal to 200. To check for all 2xx codes, I've added in statusOk. Other plugins use this as well.

Suggestions

I haven't tried this out myself but I'm happy to get it in and iterate from there. A few things I'd suggest, but that are not necessary at this stage:

Additional config verification

I'd check config.eventMethodType for example. In fact, a better choice might be to use the choice type with options like 'POST' and 'PUT', or whatever should be allowed. You could also check for leading slashes on the event path etc. Essentially anything that would cause an error to be thrown during onEvent rather than setupPlugin.

Tests

These are always nice :D

Retry queue for failed requests

This is something that recent developments in the plugin server allow for. I'm supposed to implement a few plugins using retry queues this cycle so you could use those as an example to implement it here too. You couldn't have known this was possible yet though because the docs are also coming later today.

Questions

Do you not want to send event names to Salesforce?

Copy link
Contributor

@ConradKurth ConradKurth left a comment

Choose a reason for hiding this comment

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

I like all these changes! And yeah interesting usage there for the statusok, Although I do still think the way I had it still works due to the rounding of integers, BUT regardless i like DRY'ing up that check

@ConradKurth
Copy link
Contributor

@yakkomajuri I do like your suggestions and I will make some issues for me to do (or anyone). I appreciate you taking the time to look at this and improve it!

@ConradKurth ConradKurth merged commit bc91087 into Vinovest:main May 13, 2021
@ConradKurth
Copy link
Contributor

#5 #4 #3

@yakkomajuri
Copy link
Contributor Author

yakkomajuri commented May 13, 2021

@ConradKurth unfortunately no such thing as an integer in JS (and consequently in TS).

Screenshot 2021-05-13 at 16 36 35

@yakkomajuri
Copy link
Contributor Author

Just mentioned this in Slack but another thing we should do before making this available on Cloud is certifying that salesforceHost is indeed a Salesforce URL.

@ConradKurth
Copy link
Contributor

@yakkomajuri done!

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

Successfully merging this pull request may close these issues.

2 participants