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

feat(integration-templates): [nan-1192] add checkr syncs and actions #2378

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

khaliqgant
Copy link
Member

Describe your changes

Add checkr syncs and actions:

  • GET /background-check/service-list: returns a possible list of services that Checkr can perform for a background check. Takes into account if the connection has account hierarchy enabled or not and returns the node the background check is associated with if so
  • GET /background-check/service-parameters: obtains the parameters needed to trigger a background check for a particular background check
  • POST /background-check/trigger: trigger a background check for a candidate
  • POST /candidates: create a candidate to be able to trigger background checks for

Adds a single sync:

  • GET /background-checks: retrieves all background checks for all candidates

Issue ticket number and link

NAN-1192

Checklist before requesting a review (skip if just adding/editing APIs & templates)

  • I added tests, otherwise the reason is:
  • I added observability, otherwise the reason is:
  • I added analytics, otherwise the reason is:

Copy link

linear bot commented Jun 20, 2024

Copy link
Collaborator

@TBonnin TBonnin left a comment

Choose a reason for hiding this comment

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

don't know enough about the provider to judge the business logic but code looks ok. Don't we want to systematically validate actions input with zod?

}

return { services };
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel it would be more readable if broken up into smaller functions:

export default async function runAction(nango: NangoAction): Promise<CheckrServicesResponse> {
    const { config, connection_config } = await constructRequestWithConnectionConfig(nango, '/v1/packages');
    const accountHierarchyEnabled = connection_config['accountHierarchyEnabled'] || false;

    let services: CheckrService[] = fetch({nango, config, endpoint: '/v1/packages'});

    if (accountHierarchyEnabled) {
        const nodes = await fetch({nango, config, endpoint: '/v1/nodes?include=packages'});
        services = process(nodes, services);
    }

    return { services };
}

async function fetch({nango, config, endpoint} : {nango: NangoAction, config: ProxyConfiguration, endpoint: string}) {
    const res = [];
    for await (const checkrServices of nango.paginate({ ...config, endpoint })) {
        res.push(...checkrServices);
    }
    return res;
}

function process(nodes: any[], services: CheckrService[]): CheckrService[] {
    return nodes.flatMap(node => {
        if(node.packages.length === 0) {
           return [];
        } else {
             return node.packages.map((pkg: string) => ({
                ...services.find(service => service.slug === pkg),
                node: node.custom_id
            }))
        }
    });
}

Haven't tested so details might be wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, kept as is h/e

Copy link
Collaborator

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Looks good minor comments

* This means we first fetch all the available packages. If accountHierarchyEnabled is true
* and there are nodes with packages we need to grab them from the packages to
* attach them to the node
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

imo comments are easier to understand at the exact position, I need to do a lot of back and forth to understand why it's done this way

throw new nango.ActionError({
message: `access_token is missing`
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any reason a connection would not have an acces_token?

Copy link
Member Author

Choose a reason for hiding this comment

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

More of a check to make TS happy

@@ -0,0 +1,13 @@
import type { CheckrTriggeredBackgroundCheck, BackgroundCheck } from '../../models';
Copy link
Collaborator

Choose a reason for hiding this comment

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

english: not sure about the term mappers but it's not my main language, I would call them formatters maybe

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, IMO mappers is clear

@khaliqgant khaliqgant merged commit 0945f75 into master Jun 21, 2024
20 checks passed
@khaliqgant khaliqgant deleted the nan-1192-checkr branch June 21, 2024 08:15
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.

3 participants