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

fix: dont interrupt response on display updation failure #2102

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

ShubhamPalriwala
Copy link
Member

What does this PR do?

Currently the Response Creation on Surveys block if the Display Update API throws an error even if the Response API is working fine!

Screenshot_2024-02-19-18-33-30_1920x1080

This PR fixes it by ignoring the display updation if it fails!

How should this be tested?

  • Change the API Host of the Display API & try submitting a response

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

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 Feb 19, 2024

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 Feb 22, 2024 2:37pm
formbricks-com ⬜️ Ignored (Inspect) Visit Preview Feb 22, 2024 2:37pm

Copy link
Contributor

github-actions bot commented Feb 19, 2024

Thank you for following the naming conventions for pull request titles! 🙏

Copy link
Contributor

packages/lib/responseQueue.ts

Instead of using console.log for error logging, it would be better to use a dedicated logging library that provides more flexibility and control over log levels, formats, and destinations. This will improve the readability and maintainability of your code.
Create Issue
See the diff
Checkout the fix

    import { Logger } from 'some-logging-library';

    const logger = new Logger();

    // ...

    try {
      await this.api.client.display.update(this.surveyState.displayId, {
        responseId: response.data.id,
      });
    } catch (error) {
      logger.error("Failed to update display, proceeding with the response.", error);
    }
git fetch origin && git checkout -b ReviewBot/Impro-gzwe8cm origin/ReviewBot/Impro-gzwe8cm

It's a good practice to not expose the entire error object in the logs, especially in production, as it may contain sensitive information. Instead, log only the necessary details such as the error message.
Create Issue
See the diff
Checkout the fix

    try {
      await this.api.client.display.update(this.surveyState.displayId, {
        responseId: response.data.id,
      });
    } catch (error) {
      logger.error(`Failed to update display, proceeding with the response. Error: ${error.message}`);
    }
git fetch origin && git checkout -b ReviewBot/Impro-tqgfnpz origin/ReviewBot/Impro-tqgfnpz

Comment on lines 92 to 97
await this.api.client.display.update(this.surveyState.displayId, {
responseId: response.data.id,
});
} catch (error) {
console.log("Failed to update display, proceeding with the response.", error);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The error object is being logged entirely which may expose sensitive information. The fix involves logging only the error message.

Suggested change
await this.api.client.display.update(this.surveyState.displayId, {
responseId: response.data.id,
});
} catch (error) {
console.log("Failed to update display, proceeding with the response.", error);
}
```typescript
try {
await this.api.client.display.update(this.surveyState.displayId, {
responseId: response.data.id,
});
} catch (error) {
console.log(`Failed to update display, proceeding with the response. Error: ${error.message}`);
}
```

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.

@ShubhamPalriwala thanks for the PR 😊💪

@mattinannt mattinannt added this pull request to the merge queue Feb 22, 2024
Merged via the queue into main with commit 312858e Feb 22, 2024
13 checks passed
@mattinannt mattinannt deleted the shubham/dont-interrupt-response-if-display-fails branch February 22, 2024 14:54
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.

None yet

2 participants