keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
208 stars 105 forks source link

WEB: suggesting changes while adding waitlist #463

Closed HarshPatel5940 closed 1 month ago

HarshPatel5940 commented 1 month ago

Description


Target file: https://github.com/keyshade-xyz/keyshade/blob/72281e66569b05011a7b79a94ae502eaac0b3a6d/apps/web/src/components/hero/index.tsx

rajdip-b commented 1 month ago

@kriptonian1 can you review this once? If valid, please open up 4 separate issues.

Nil2000 commented 1 month ago

For the 4th case, If anyone try to hit the end point using the same email, the response says Successfully subscribed. It should either say Already subscribed or give error at the response. May be we require DB to tackle this. Or may be add some settings in mailchimp🤔

HarshPatel5940 commented 1 month ago

For the 4th case, If anyone try to hit the end point using the same email, the response says Successfully subscribed. It should either say Already subscribed or give error at the response. May be we require DB to tackle this. Or may be add some settings in mailchimp🤔

As far as i remember to tackle that, we need to hash the email and check if the mail exist in the mailing list. That would be another request from our side as much as i remember. Need to explore myself.

Alternatively we have https://listmonk.app/ just wanted to throw this in cause its a cool mailing list manager which is easy to use.

kriptonian1 commented 1 month ago

For the 4th case, If anyone try to hit the end point using the same email, the response says Successfully subscribed. It should either say Already subscribed or give error at the response. May be we require DB to tackle this. Or may be add some settings in mailchimp🤔

I am not sure if we can get any such response from Mailchimp or not

kriptonian1 commented 1 month ago

For the 4th case, If anyone try to hit the end point using the same email, the response says Successfully subscribed. It should either say Already subscribed or give error at the response. May be we require DB to tackle this. Or may be add some settings in mailchimp🤔

As far as i remember to tackle that, we need to hash the email and check if the mail exist in the mailing list. That would be another request from our side as much as i remember. Need to explore myself.

Alternatively we have https://listmonk.app/ just wanted to throw this in cause its a cool mailing list manager which is easy to use.

Actually we are using Mailchimp because this is what our marketing team is using, not sure if we can do campaigns through listmonk

HarshPatel5940 commented 1 month ago

Actually we are using Mailchimp because this is what our marketing team is using, not sure if we can do campaigns through listmonk

Hmm, got it. 👍🏼

kriptonian1 commented 1 month ago
image

@HarshPatel5940 I think in 2nd point this what you mentioned, right?

HarshPatel5940 commented 1 month ago
image

@HarshPatel5940 I think in 2nd point this what you mentioned, right?

No i meant it allows ff@ff ... it should not allow that right.. instead ff@ff.com can be allowed ig.

bansal-harsh-2504 commented 1 month ago

@kriptonian1 can I contribute to this issue please?

kriptonian1 commented 1 month ago

@bansal-harsh-2504 sure

bansal-harsh-2504 commented 1 month ago

/attempt

github-actions[bot] commented 1 month ago

Assigned the issue to @bansal-harsh-2504!

bansal-harsh-2504 commented 1 month ago

@kriptonian1 @rajdip-b Do I need to rebuild using Docker after each file change? It takes around 300 seconds for each build. This is my first time using Docker, so please bear with me.

and can you please open four seperate issues?

rajdip-b commented 1 month ago

I don't think you need docker in the first place. Docker is for self-hosting it (partly). You should be using the pnpm commands for this ideally.

Do go through our docs to understand how to get this done:

bansal-harsh-2504 commented 1 month ago

@rajdip-b @kriptonian1 I am done with (2.[chore] Perform some validation on the email field as ff@ff and other things is allowed which should not be allowed.) So do i need to create pr for this issue or you guys are creating seperate issues.

I want to ask one thing, do we need to allow all email or those ending with .com

rajdip-b commented 1 month ago

We don't want to impose any restriction on the domain. Just check for this regex: .[\w]{2,}

bansal-harsh-2504 commented 1 month ago

@rajdip-b Could you please share the Mailchimp API URL to check if an email has already been waitlisted? Appreciate your help!

poswalsameer commented 1 month ago

Hi @rajdip-b, I have got the issue and am done with almost half of the code changes. Please assign this to me, so that I can start working on this issue. Thanks!

rajdip-b commented 1 month ago

Hey @poswalsameer, Harsh has already dropped 2 PRs for the first 2 points. I can assign this to you along with him, but I would recommend you to not work on the features that Harsh has already worked on.

poswalsameer commented 1 month ago

Hey @poswalsameer, Harsh has already dropped 2 PRs for the first 2 points. I can assign this to you along with him, but I would recommend you to not work on the features that Harsh has already worked on.

Hi, thanks for assigning the issue. Yes, I have seen the PRs dropped for the first 2 points, will be working on the other two.

bansal-harsh-2504 commented 1 month ago

@rajdip-b The third point is unnecessary and can be disregarded.

poswalsameer commented 1 month ago

@rajdip-b The third point is unnecessary and can be disregarded.

Hi, thanks for letting me know about this. Will take care of this thing.

rajdip-b commented 1 month ago

Special thanks to @HarshPatel5940 for bug hunting <3

poswalsameer commented 1 month ago

Hi @rajdip-b, raised PR #483 to solve the issue mentioned in point 4. The issue mentioned in point 3 is not significant and has already been taken care of by the current codebase. Please review this PR so it can get merged. Thank You!

HarshPatel5940 commented 1 month ago

Special thanks to @HarshPatel5940 for bug hunting <3

Happy to help!

HarshPatel5940 commented 1 month ago

Hi @rajdip-b, raised PR #483 to solve the issue mentioned in point 4. The issue mentioned in point 3 is not significant and has already been taken care of by the current codebase. Please review this PR so it can get merged. Thank You!

Hi Sameer, Point 3 is actually valid here cause I remember turning off the wifi and still getting a successfully added to the waitlist even when the request failed due to no network.

If you can verify that this situation is not happening, it would be helpful.

poswalsameer commented 1 month ago

Hi @rajdip-b, raised PR #483 to solve the issue mentioned in point 4. The issue mentioned in point 3 is not significant and has already been taken care of by the current codebase. Please review this PR so it can get merged. Thank You!

Hi Sameer, Point 3 is actually valid here cause I remember turning off the wifi and still getting a successfully added to the waitlist even when the request failed due to no network.

If you can verify that this situation is not happening, it would be helpful.

Thanks for pointing out the exact issue. I will start working on this, till then you can review the code for point 4.

HarshPatel5940 commented 1 month ago

Thanks for pointing out the exact issue. I will start working on this, till then you can review the code for point 4.n

mhmm, noted.

bansal-harsh-2504 commented 1 month ago

Hi @rajdip-b, raised PR #483 to solve the issue mentioned in point 4. The issue mentioned in point 3 is not significant and has already been taken care of by the current codebase. Please review this PR so it can get merged. Thank You!

Hi Sameer, Point 3 is actually valid here cause I remember turning off the wifi and still getting a successfully added to the waitlist even when the request failed due to no network.

If you can verify that this situation is not happening, it would be helpful.

Thank you for pointing that out. After reviewing the code, I realize you are correct.

I am done with the solution. @rajdip-b @poswalsameer May I raise a PR for point 3?

HarshPatel5940 commented 1 month ago

I am done with the solution. @rajdip-b @poswalsameer May I raise a PR for point 3?

Feel free to do it :D

poswalsameer commented 1 month ago

Hi @rajdip-b, raised PR #483 to solve the issue mentioned in point 4. The issue mentioned in point 3 is not significant and has already been taken care of by the current codebase. Please review this PR so it can get merged. Thank You!

Hi Sameer, Point 3 is actually valid here cause I remember turning off the wifi and still getting a successfully added to the waitlist even when the request failed due to no network. If you can verify that this situation is not happening, it would be helpful.

Thank you for pointing that out. After reviewing the code, I realize you are correct.

I am done with the solution. @rajdip-b @poswalsameer May I raise a PR for point 3?

Sure, go for it.

rajdip-b commented 1 month ago

Impressive work guys!

rajdip-b commented 3 weeks ago

:tada: This issue has been resolved in version 2.6.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: