Update Hubspot Credential model #230

Jo-lefthook · 2023-12-05T23:42:28Z

Update Credential model columns to be snake_case
Update manager to use externalId on Credential instead of portalId Add setCredential function
Update redirect_uri

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @friggframework/[email protected]
# or 
yarn add @friggframework/[email protected]

Update Credential model columns to be snake_case Update manager to use externalId on Credential instead of portalId Add setCredential function Update redirect_uri

ghost · 2023-12-05T23:43:14Z

👇 Click on the image for a new way to code review

Legend

seanspeaks

See my notes. Some questions/answers may change a bit of my suggestions but you'll see my primary concern is on the state and redirect uri params. @MichaelRyanWebber's leanings for v1 will impact what I think we should put here for the pre-v1 release.

seanspeaks · 2023-12-06T00:04:05Z

api-module-library/hubspot/manager.js

            delegate: instance,
+            state: JSON.stringify({ app: 'hubspot' })


I think this one we want to pass in whatever state arg is provided by the caller for getAuthRequirements (this is not quite fully supported in concept yet but it's the logical place to add support) and remove the hardcoded json string here (it's a vestige of how FreshBooks' app was built)

(Then it's on us to pass it in on the get authorize route in our local Frigg application)

@seanspeaks The thing is, the authorizationUri is set in the constructor which prevents it from being updated later in the getAuthRequirements call. So we can either:
1- update the getAuthUri to read the state on the fly, similar to slack
2- accept the api state via the params of getInstance

Thoughts?

@Jo-lefthook IMO the best way here is to change the getAuthUri() to re-generate the authorizationUri with the latest variable updates

seanspeaks · 2023-12-06T00:08:03Z

api-module-library/hubspot/manager.js

@@ -31,8 +31,9 @@ class Manager extends ModuleManager {
            client_id: process.env.HUBSPOT_CLIENT_ID,
            client_secret: process.env.HUBSPOT_CLIENT_SECRET,
            scope: process.env.HUBSPOT_SCOPE,
-            redirect_uri: `${process.env.REDIRECT_URI}/hubspot`,
+            redirect_uri: process.env.REDIRECT_URI,


This one is tricky... @MichaelRyanWebber do you have an opinion on how we should navigate this? Each Frigg adopter may have a slightly different way they want to handle OAuth redirects, so if we're going to default to our "base url + api name" convention, we should have an easy override.

Overriding seems like a not so simple thing to do in Frigg v1 unless we have it at the module options level the integration definition?

In the current FreshBooks implementation, @Jo-lefthook, we should probably override the getInstance method in the local HubSpotEntityManager and change the redirect argument there instead of here in the published API Module

@seanspeaks Yea, maybe we can start introducing an env variable for each integration library. for example, redirect_uri here would be something like process.env.FRIGG_HUBSPOT_REDIRECT_URI || ${process.env.REDIRECT_URI}/hubspot

@Jo-lefthook @seanspeaks I think we should follow the same pattern as Microsoft Teams, check it out here:
https://github.com/friggframework/frigg/blob/main/api-module-library/microsoft-teams/manager.js

It allows the user to override the redirect_uri, and if not it gets the default one ${process.env.REDIRECT_URI}/hubspot already using Frigg's helpers

We could follow the Microsoft Teams style in v1.

In v1 we can override the definition within the integration (or elsewhere and import from there).

We could also have each API Module support an optional env variable like process.env.REDIRECT_URI_<MODULE_NAME> and if that's not there, use the global style:
${process.env.REDIRECT_URI}/<module-name>

Cool, so we agree on the Microsoft Teams approach. I've pushed a commit implementing that. For the env variable, as @MichaelRyanWebber said, I think this should be added to the pre-release checklist to be implemented in all API modules

leofmds

I think we should at least clarify the naming changes before moving on.

leofmds · 2023-12-06T12:46:07Z

api-module-library/hubspot/models/credential.js

What's the reason to change the variable names? This would be a breaking change, so it should be updated only if really needed.

leofmds · 2023-12-06T12:48:01Z

api-module-library/hubspot/manager.js

                    auth_is_valid: true,
                };

                if (!this.credential) {
                    let credentialSearch = await Credential.find({
-                        portalId: userDetails.portalId,
+                        externalId: userDetails.portalId,


This is also a breaking change but makes sense, so 👍🏻

leofmds · 2023-12-06T12:51:15Z

api-module-library/hubspot/manager.js

@@ -41,8 +42,8 @@ class Manager extends ModuleManager {
                instance.entity.credential
            );
            instance.credential = credential;
-            apiParams.access_token = credential.accessToken;
-            apiParams.refresh_token = credential.refreshToken;
+            apiParams.access_token = credential.access_token;


Also related to a breaking change. I think we should start following a standardized naming convention for the names, instead of mixing camel case and snake case. I'm in favor of changing apiParams.access_token to apiParams.accessToken instead of changing the credential variables (see comment below).

The v1 style allows for just saving params as they exist on the API class, rather than converting them on storage and then back on retrieval.

I think we should follow the way the tokens are presented and used by the external API and Oauth2. And then the credential's job of persistence doesn't require a transformation.

leofmds · 2023-12-06T12:52:48Z

api-module-library/hubspot/package.json

@@ -1,6 +1,6 @@
 {
    "name": "@friggframework/api-module-hubspot",
-    "version": "0.9.1",
+    "version": "0.10.0",


This PR contains breaking changes, the version should be bumped to 1.0.0 at least. Still, a question mark if this would be ready for a v1.

sonarcloud · 2023-12-07T21:04:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

Hubspot:

1623370

Update Credential model columns to be snake_case Update manager to use externalId on Credential instead of portalId Add setCredential function Update redirect_uri

Jo-lefthook requested a review from seanspeaks December 5, 2023 23:42

seanspeaks requested changes Dec 6, 2023

View reviewed changes

leofmds suggested changes Dec 6, 2023

View reviewed changes

Jo-lefthook added 4 commits December 6, 2023 12:14

Support setting redirect_uri via husbpot manager constructor

4b640b4

Update getAuthUri to return the uri on the fly

1f99b6f

update version to 1.1

3675833

Change version to 1.0.0

034cdd2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Hubspot Credential model #230

Update Hubspot Credential model #230

Jo-lefthook commented Dec 5, 2023 •

edited by seanspeaks

Loading

ghost commented Dec 5, 2023 •

edited by ghost

Loading

Legend

seanspeaks left a comment

seanspeaks Dec 6, 2023

Jo-lefthook Dec 6, 2023

leofmds Dec 6, 2023

seanspeaks Dec 6, 2023

Jo-lefthook Dec 6, 2023 •

edited

Loading

leofmds Dec 6, 2023

MichaelRyanWebber Dec 6, 2023

Jo-lefthook Dec 6, 2023

leofmds left a comment

leofmds Dec 6, 2023

leofmds Dec 6, 2023

leofmds Dec 6, 2023

MichaelRyanWebber Dec 6, 2023

leofmds Dec 6, 2023

sonarcloud bot commented Dec 7, 2023

		delegate: instance,
		state: JSON.stringify({ app: 'hubspot' })

Update Hubspot Credential model #230

Are you sure you want to change the base?

Update Hubspot Credential model #230

Conversation

Jo-lefthook commented Dec 5, 2023 • edited by seanspeaks Loading

ghost commented Dec 5, 2023 • edited by ghost Loading

Legend

seanspeaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jo-lefthook Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leofmds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 7, 2023

Jo-lefthook commented Dec 5, 2023 •

edited by seanspeaks

Loading

ghost commented Dec 5, 2023 •

edited by ghost

Loading

Jo-lefthook Dec 6, 2023 •

edited

Loading