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: the set as default apps issue with links #19176 #19198

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

PAVANNAIK25
Copy link

What does this PR do?

Loom Video:
https://www.loom.com/share/0f6e8a0561174216abba9e22bc23035b?sid=39a5a0ad-cc1a-4988-abe4-153735ac32c3

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox. N/A
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Are there environment variables that should be set?
    • No.
  • What are the minimal test data to have?
    • Installed apps conferencing should have Mirotalk installed.
  • What is expected (happy path) to have (input and output)?
  • I/P: set the default app as Mirotalk. O/P it should be set to default.
  • Any other important info that could help to test that PR
    • Issue was regarding conference apps set as default. Apps which needs the links to set as default (example: mirotalk, discord, etc) was throwing error as "App link is required". This PR fixes the issue and fixed the issue with set as default.

@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Feb 8, 2025
@graphite-app graphite-app bot requested a review from a team February 8, 2025 20:25
@github-actions github-actions bot added Low priority Created by Linear-GitHub Sync 🐛 bug Something isn't working labels Feb 8, 2025
Copy link

vercel bot commented Feb 8, 2025

Someone is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

graphite-app bot commented Feb 8, 2025

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (02/08/25)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (02/08/25)

1 label was added to this PR based on Keith Williams's automation.

"Add ready-for-e2e label" took an action on this PR • (02/10/25)

1 label was added to this PR based on Keith Williams's automation.

@retrogtx
Copy link
Contributor

Hey @PAVANNAIK25

Thank you for the PR, it works. Could you please also implement the period and underscore check for links so that they get allowed?

@PAVANNAIK25
Copy link
Author

Hi @retrogtx,

I have noticed that the issue affects all apps that require a link to be set as default. The current regex validation only allows uppercase letters, lowercase letters, and numbers, but it does not permit (.) or (_). Additionally, I would like to clarify whether we should also allow (-), as apps like Mirotalk allow users to create links with (-) as well. Should I include (-) in the validation?

@retrogtx
Copy link
Contributor

Hi @retrogtx,

I have noticed that the issue affects all apps that require a link to be set as default. The current regex validation only allows uppercase letters, lowercase letters, and numbers, but it does not permit (.) or (_). Additionally, I would like to clarify whether we should also allow (-), as apps like Mirotalk allow users to create links with (-) as well. Should I include (-) in the validation?

please do, I will test the changes then

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Pavan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@retrogtx
Copy link
Contributor

thank you tonnes, can you update the loom video for the same with the - examples?

@PAVANNAIK25
Copy link
Author

thank you tonnes, can you update the loom video for the same with the - examples?

Loom Video:
https://www.loom.com/share/0469cc33131e4fe5ad893b5f3b73e35f?sid=a79172a1-2053-4b22-ab6e-7d8c47c5bc61

Copy link
Contributor

@retrogtx retrogtx left a comment

Choose a reason for hiding this comment

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

tested, works

@retrogtx
Copy link
Contributor

thank you tonnes, can you update the loom video for the same with the - examples?

Loom Video: https://www.loom.com/share/0469cc33131e4fe5ad893b5f3b73e35f?sid=a79172a1-2053-4b22-ab6e-7d8c47c5bc61

please sign the CLA so that we can merge this 🙏

@PAVANNAIK25
Copy link
Author

please sign the CLA so that we can merge this 🙏

I have already singed it. Please merge this request.

image

Copy link
Contributor

E2E results are ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working community Created by Linear-GitHub Sync Low priority Created by Linear-GitHub Sync ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-5137] Mirotalk app not recognizing Link
3 participants