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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Airtable & GS endpoints to use responses return objects #1592

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

Dhruwang
Copy link
Contributor

@Dhruwang Dhruwang commented Nov 7, 2023

What does this PR do?

Fixes 1458

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change adds a new database migration
  • This change requires a documentation update

How should this be tested?

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read How we Code at Formbricks
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues
  • First PR at Formbricks? Please sign the CLA! Without it we wont be able to merge it 馃檹

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

Copy link

vercel bot commented Nov 7, 2023

The latest updates on your projects. Learn more about Vercel for Git 鈫楋笌

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
formbricks-cloud 猬滐笍 Ignored (Inspect) Visit Preview Nov 7, 2023 1:20pm
formbricks-com 猬滐笍 Ignored (Inspect) Nov 7, 2023 1:20pm

Copy link
Contributor

github-actions bot commented Nov 7, 2023

Thank you for following the naming conventions for pull request titles! 馃檹

Copy link
Contributor

responses.js

It's a good practice to centralize the error handling and response generation in APIs. This will make the code more maintainable and readable. In the PR, the error handling and response generation are scattered across different routes. It would be better to create a separate utility for generating responses and handling errors.
Create Issue

    // Create a new file responses.js
    export const responses = {
      successResponse: (data) => {
        return NextResponse.json({ data }, { status: 200 });
      },
      badRequestResponse: (message) => {
        return NextResponse.json({ error: message }, { status: 400 });
      },
      notAuthenticatedResponse: () => {
        return NextResponse.json({ error: "Invalid session" }, { status: 400 });
      },
      unauthorizedResponse: () => {
        return NextResponse.json({ error: "You don't have access to environment" }, { status: 401 });
      },
      notFoundResponse: (message, data) => {
        return NextResponse.json({ error: message, data }, { status: 404 });
      },
      internalServerErrorResponse: (message) => {
        return NextResponse.json({ error: message }, { status: 500 });
      },
    };

integration/service.js

The creation and update of the integration are repeated in the code. It would be better to create a separate function for this operation to avoid code duplication and enhance readability.
Create Issue

    // Create a new function in the integration service
    export const createOrUpdateIntegration = async (environmentId, integration) => {
      return await prisma.integration.upsert({
        where: {
          type_environmentId: {
            environmentId,
            type: integration.type,
          },
        },
        update: {
          ...integration,
          environment: { connect: { id: environmentId } },
        },
        create: {
          ...integration,
          environment: { connect: { id: environmentId } },
        },
      });
    };

Multiple files

The variable name AIR_TABLE_CLIENT_ID is not consistent with the other variable names. It would be better to rename it to AIRTABLE_CLIENT_ID for consistency.
Create Issue

    // Replace all occurrences of AIR_TABLE_CLIENT_ID with AIRTABLE_CLIENT_ID
    const client_id = AIRTABLE_CLIENT_ID;

airtable.ts

The fetchTables function is currently returning the entire response object. It would be better to return only the data property of the response to reduce the amount of data being transferred.
Create Issue

    // Refactor the fetchTables function
    export const fetchTables = async (environmentId: string, baseId: string) => {
      const res = await fetch(`/api/v1/integrations/airtable/tables?baseId=${baseId}`, {
        method: "GET",
        headers: { environmentId: environmentId },
        cache: "no-store",
      });
      const resJson = await res.json();
      return resJson.data as Promise<TIntegrationAirtableTables>;
    };

Copy link
Member

@mattinannt mattinannt left a comment

Choose a reason for hiding this comment

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

@Dhruwang thank you for the changes 馃槉馃檹

@mattinannt mattinannt added this pull request to the merge queue Nov 9, 2023
Merged via the queue into main with commit dd0f0ea Nov 9, 2023
14 checks passed
@mattinannt mattinannt deleted the integration-refactor branch November 9, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants