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

WEB: suggesting changes while adding waitlist #463

Closed
4 tasks done
HarshPatel5940 opened this issue Sep 24, 2024 · 33 comments · Fixed by #492
Closed
4 tasks done

WEB: suggesting changes while adding waitlist #463

HarshPatel5940 opened this issue Sep 24, 2024 · 33 comments · Fixed by #492
Assignees
Labels
difficulty: 2 hacktoberfest Hacktoberfest is on! priority: medium released scope: web Everything related to the webiste type: bug Something isn't working

Comments

@HarshPatel5940
Copy link
Contributor

HarshPatel5940 commented Sep 24, 2024

Description


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

@rajdip-b rajdip-b changed the title web: suggesting changes while adding whitelist WEB: suggesting changes while adding waitlist Sep 24, 2024
@rajdip-b
Copy link
Member

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

@Nil2000
Copy link
Contributor

Nil2000 commented Sep 25, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

kriptonian1 commented Sep 30, 2024

image

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

@HarshPatel5940
Copy link
Contributor Author

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 [email protected] can be allowed ig.

@bansal-harsh-2504
Copy link
Contributor

bansal-harsh-2504 commented Oct 1, 2024

@kriptonian1 can I contribute to this issue please?

@kriptonian1
Copy link
Contributor

@bansal-harsh-2504 sure

@bansal-harsh-2504
Copy link
Contributor

/attempt

Copy link

github-actions bot commented Oct 2, 2024

Assigned the issue to @bansal-harsh-2504!

@bansal-harsh-2504
Copy link
Contributor

bansal-harsh-2504 commented Oct 2, 2024

@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
Copy link
Member

rajdip-b commented Oct 2, 2024

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
Copy link
Contributor

bansal-harsh-2504 commented Oct 2, 2024

@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
Copy link
Member

rajdip-b commented Oct 3, 2024

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

@rajdip-b rajdip-b added type: bug Something isn't working scope: web Everything related to the webiste priority: medium difficulty: 2 hacktoberfest Hacktoberfest is on! labels Oct 3, 2024
@bansal-harsh-2504
Copy link
Contributor

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

@poswalsameer
Copy link
Contributor

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
Copy link
Member

rajdip-b commented Oct 8, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@poswalsameer
Copy link
Contributor

@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
Copy link
Member

rajdip-b commented Oct 9, 2024

Special thanks to @HarshPatel5940 for bug hunting <3

@poswalsameer
Copy link
Contributor

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
Copy link
Contributor Author

Special thanks to @HarshPatel5940 for bug hunting <3

Happy to help!

@HarshPatel5940
Copy link
Contributor Author

HarshPatel5940 commented Oct 9, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

Feel free to do it :D

@poswalsameer
Copy link
Contributor

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
Copy link
Member

Impressive work guys!

@rajdip-b
Copy link
Member

🎉 This issue has been resolved in version 2.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment