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: Prevent incomplete submissions and defer captcha verification #92

Merged
merged 7 commits into from
Sep 1, 2024

Conversation

narcode
Copy link
Collaborator

@narcode narcode commented Aug 15, 2024

Related Issues
Fixes #91 and adds input verification before allowing for moment submission

Proposed Changes (Optional)

  • reactively disable the "Add" button if the required payload is incomplete
  • defer the captcha verification until the required inputs are fulfilled and the user clicks "Add moment"

Additional context (Optional)

@narcode narcode self-assigned this Aug 15, 2024
@narcode narcode linked an issue Aug 15, 2024 that may be closed by this pull request
@narcode narcode requested a review from jokroese August 15, 2024 21:55
Copy link

netlify bot commented Aug 15, 2024

Deploy Preview for qtmmirror ready!

Name Link
🔨 Latest commit 5333c79
🔍 Latest deploy log https://app.netlify.com/sites/qtmmirror/deploys/66ca3dba35d6ed0008850725
😎 Deploy Preview https://deploy-preview-92--qtmmirror.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@jokroese jokroese left a comment

Choose a reason for hiding this comment

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

These changes enable the add button only when users hover over the add button. As hovering is only possible on desktop, these changes would block mobile users from being able to submit moments.

Instead, the core functionality should be tied to clicking which is possible on all devices.

@jokroese
Copy link
Member

I suggest:

  1. Deriving the isAddButtonDisabled boolean reactively. (Currently it is tied to hovering over the add button).
  2. Requesting the captcha on clicking the add button.

@narcode
Copy link
Collaborator Author

narcode commented Aug 19, 2024

Oh yeah! I completly forgot about mobile. Great catch! Will process the changes you suggest! 👍

@narcode narcode requested a review from jokroese August 19, 2024 20:59
@narcode
Copy link
Collaborator Author

narcode commented Aug 19, 2024

@jokroese I have updated the PR from your comments. Super nice and swifty the $: reactive statements in svelte! didn't know it and I am positively surprised :)

@narcode narcode force-pushed the 91-error-captcha-verification-failed branch from b3e8115 to 3ec6e92 Compare August 19, 2024 21:05
@jokroese jokroese linked an issue Aug 19, 2024 that may be closed by this pull request
@Elie-Simard
Copy link
Contributor

Elie-Simard commented Aug 20, 2024

I see #76 just got resolve? Else we could also add this to src/routes/moments/+server.ts on line 12

  // Empty description check
  if (!description || description.trim() === '') {
    return json({ error: 'Description cannot be empty.' }, { status: 400 });
  } 

@narcode
Copy link
Collaborator Author

narcode commented Aug 20, 2024

I see #76 just got resolve? Else we could also add this to src/routes/moments/+server.ts on line 12

  // Empty description check
  if (!description || description.trim() === '') {
    return json({ error: 'Description cannot be empty.' }, { status: 400 });
  } 

With the latest changes that disable/enable the AddButton reactively we can be confident that empty descriptions cannot reach the server. Only when the description is present (plus the point coordinates) can the moment be submitted.

I would assume the PR as it is now should be fine and #76 can be considered fixed but if the general consensus is to add the server guard i’m happy to add it too.

@narcode
Copy link
Collaborator Author

narcode commented Aug 24, 2024

@jokroese @Elie-Simard I thought thoroughly about @Elie-Simard comment regarding #76 and it is not overkill to also add the check at the server level.

This however made me realize that the real solution is to improve the data model at DB level and add a migration to that makes descritpions or coordinates NOT NULL. This is definetly out of the scope of this ticket and we could make a new issue for revisiting the DB model.

Copy link
Member

@jokroese jokroese left a comment

Choose a reason for hiding this comment

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

Simple, elegant, beautiful.

@jokroese jokroese merged commit 9b99e60 into main Sep 1, 2024
7 checks passed
@jokroese jokroese deleted the 91-error-captcha-verification-failed branch September 1, 2024 18:47
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.

Error: CAPTCHA verification failed. dev - add rule to ADD that there must be text content
3 participants