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

Feature/invalid credentials #5120

Merged
merged 13 commits into from
Oct 31, 2022
Merged

Feature/invalid credentials #5120

merged 13 commits into from
Oct 31, 2022

Conversation

alannnc
Copy link
Contributor

@alannnc alannnc commented Oct 19, 2022

What does this PR do?

Adds a way to invalidate credentials, so we can now let the user now when an integration has broken.

  • Add migration that creates new field on Credential table, invalid.
  • Display alert error message on UI when credentials has been flag as invalid.
  • When certain authentication response is received for zoom we flag that credential as invalid.
  • Fix a lot of types from Prisma with the correct interface.

image

Environment: Staging(main branch)

Type of change

  • Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

  • Install any app and from the DB update its invalid field to true.
  • Now in the installed app page we should receive a alert error message, so our user can manually reinstall again.

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

@vercel
Copy link

vercel bot commented Oct 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ❌ Failed (Inspect) Oct 31, 2022 at 8:05PM (UTC)


import appStore from "..";

const log = logger.getChildLogger({ prefix: ["CalendarManager"] });

export const getCalendar = (credential: Credential | null): Calendar | null => {
export const getCalendar = (credential: CredentialPayload | null): Calendar | null => {
Copy link
Contributor Author

@alannnc alannnc Oct 28, 2022

Choose a reason for hiding this comment

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

Most of the changes includes changing the type that was being used for Credential that isn't allowing for optional fields.

if (responseBody.error === "invalid_grant") {
return Promise.reject(new Error("Invalid grant for Cal.com zoom app"));
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If invalid_grant is received we should invalidate the credential right away.

throw Error(response.statusText);
}

return response.json();
};

const invalidateCredential = async (credentialId: Credential["id"]) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our function that invalidate certain credential

};
}>;

export type CredentialWithAppName = CredentialPayload & { appName: string };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A help type definition to use when requiring Credential Type from DB.

@alannnc alannnc marked this pull request as ready for review October 28, 2022 19:24
@alannnc alannnc requested a review from a team October 28, 2022 19:25
@alannnc alannnc changed the title [WIP]Feature/invalid credentials Feature/invalid credentials Oct 28, 2022
Copy link
Contributor

@leog leog left a comment

Choose a reason for hiding this comment

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

LGTM, great job, very useful for the users!

I was just wondering, is this a retroactive change for existing credentials? If not, would it be worth in executing some script? Also, perhaps it would be worth exploring running such a script with an email notification added, so users can be aware their apps will not work as expected, before anyone books a new event and they don't get the desired behavior out of their installed apps.

On another note, as a user I would prefer the app facilitates an action to let me delete and reinstall all in one, instead of providing a delete button to then having to go search the app again and reinstall it. Within this context, we know the app already, we could just add a delete & reinstall alongside the delete button, WDYT @Jaibles?

@alannnc
Copy link
Contributor Author

alannnc commented Oct 28, 2022

I was just wondering, is this a retroactive change for existing credentials? If not, would it be worth in executing some script? Also, perhaps it would be worth exploring running such a script with an email notification added, so users can be aware their apps will not work as expected, before anyone books a new event and they don't get the desired behaviour out of their installed apps.

About retroactive this should work with existing credential, we just missing a way to invalidate them. (Zoom its the only one right now).
Also for notifications and such, we should add:

  • Notification via backend that something broke.

On another note, as a user I would prefer the app facilitates an action to let me delete and reinstall all in one, instead of providing a delete button to then having to go search the app again and reinstall it.
This should be reinstall button you're right there we can do that of course, but probably next iteration.

@alannnc alannnc added the ❗️ migrations contains migration files label Oct 28, 2022
@alannnc alannnc requested a review from emrysal October 31, 2022 20:24
@PeerRich PeerRich merged commit ac9b2d0 into main Oct 31, 2022
@PeerRich PeerRich deleted the feature/invalid-credentials branch October 31, 2022 22:06
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
* Fixing types from handleErrorJson usage and Credential

* Replace credential prisma type for a better suitable

* Improvements on zoom video adapter

* Renamed extendedCredentialType and put it in a best suited file

* Frontend display invalid credential

* Fix styles and text

* Fix type required for fake daily credentials
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
* Fixing types from handleErrorJson usage and Credential

* Replace credential prisma type for a better suitable

* Improvements on zoom video adapter

* Renamed extendedCredentialType and put it in a best suited file

* Frontend display invalid credential

* Fix styles and text

* Fix type required for fake daily credentials
@PeerRich PeerRich added the core area: core, team members only label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core area: core, team members only ❗️ migrations contains migration files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants