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

Send mobile editor email #634

Merged
merged 2 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions api/app/Http/Controllers/Forms/FormController.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use App\Http\Resources\FormResource;
use App\Models\Forms\Form;
use App\Models\Workspace;
use App\Notifications\Forms\MobileEditorEmail;
use App\Service\Forms\FormCleaner;
use App\Service\Storage\StorageFileNameParser;
use Illuminate\Support\Facades\Auth;
Expand Down Expand Up @@ -271,4 +272,16 @@ public function updateWorkspace($id, $workspace_id)
'message' => 'Form workspace updated successfully.',
]);
}

public function mobileEditorEmail($id)
{
$form = Form::findOrFail($id);
$this->authorize('update', $form);

$form->creator->notify(new MobileEditorEmail($form->slug));

return $this->success([
'message' => 'Email sent.',
]);
}
Comment on lines +276 to +286
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Based on the findings, I can now generate the final response:

Add rate limiting and error handling to protect email notifications

The review suggestions are valid as:

  1. The MobileEditorEmail notification is queued (implements ShouldQueue) but lacks error handling
  2. No rate limiting is currently applied to the endpoint
  3. The notification implementation exists but could benefit from better error handling
+    /**
+     * Send a mobile editor notification email to the form creator.
+     *
+     * @param int $id The form ID
+     * @return \Illuminate\Http\JsonResponse
+     * @throws \Illuminate\Auth\Access\AuthorizationException
+     */
    public function mobileEditorEmail($id)
    {
+        $this->middleware('throttle:3,10');

        $form = Form::findOrFail($id);
        $this->authorize('update', $form);

+        try {
            $form->creator->notify(new MobileEditorEmail($form->slug));
+            return $this->success(['message' => 'Email sent successfully.']);
+        } catch (\Exception $e) {
+            \Log::error('Failed to send mobile editor email', [
+                'form_id' => $id,
+                'error' => $e->getMessage()
+            ]);
+            return $this->error(['message' => 'Failed to send email. Please try again later.']);
+        }
-        return $this->success(['message' => 'Email sent.']);
    }
🔗 Analysis chain

Add rate limiting, error handling, and documentation.

While the basic implementation is functional, several improvements are recommended:

  1. Add rate limiting to prevent potential abuse
  2. Implement error handling for notification failures
  3. Add PHPDoc to document the method's purpose and parameters

Consider applying these changes:

+    /**
+     * Send a mobile editor notification email to the form creator.
+     *
+     * @param int $id The form ID
+     * @return \Illuminate\Http\JsonResponse
+     * @throws \Illuminate\Auth\Access\AuthorizationException
+     */
+    public function mobileEditorEmail($id)
+    {
+        $this->middleware('throttle:3,10'); // Limit to 3 requests per 10 minutes
+
+        $form = Form::findOrFail($id);
+        $this->authorize('update', $form);
+
+        try {
+            $form->creator->notify(new MobileEditorEmail($form->slug));
+
+            return $this->success([
+                'message' => 'Email sent successfully.',
+            ]);
+        } catch (\Exception $e) {
+            \Log::error('Failed to send mobile editor email', [
+                'form_id' => $id,
+                'error' => $e->getMessage()
+            ]);
+
+            return $this->error([
+                'message' => 'Failed to send email. Please try again later.',
+            ]);
+        }
+    }

Additionally:

Let's verify the notification implementation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if MobileEditorEmail implements proper interfaces and methods
ast-grep --pattern 'class MobileEditorEmail {
  $$$
  public function via($notifiable) {
    $$$
  }
  public function toMail($notifiable) {
    $$$
  }
  $$$
}'

Length of output: 163


Script:

#!/bin/bash
# Let's first locate the MobileEditorEmail notification class
fd MobileEditorEmail

# Then check its implementation
rg -A 10 "class MobileEditorEmail" 

# Also check if there's any rate limiting middleware already defined
rg -A 5 "throttle.*minutes"

Length of output: 131979


Script:

#!/bin/bash
# Let's check if there's any rate limiting middleware already defined in the controller
rg -A 5 "middleware.*throttle" api/app/Http/Controllers/Forms/FormController.php

# Check if there's any error handling for notifications in the codebase
rg -A 5 "try.*notify.*catch" api/app/Http/Controllers/Forms/FormController.php

# Check if MobileEditorEmail notification class implements proper interfaces
rg -A 10 "class MobileEditorEmail" api/app/Notifications/Forms/MobileEditorEmail.php

Length of output: 467


Script:

#!/bin/bash
# Let's check if there's any global rate limiting middleware applied
rg -A 5 "throttle" api/routes/web.php api/routes/api.php

# Check the implementation of MobileEditorEmail notification to understand its behavior
rg -A 20 "class MobileEditorEmail" api/app/Notifications/Forms/MobileEditorEmail.php

# Check if there's any error handling in the base controller or traits
rg -A 5 "try.*notify.*catch" api/app/Http/Controllers/Controller.php

Length of output: 675

}
53 changes: 53 additions & 0 deletions api/app/Notifications/Forms/MobileEditorEmail.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php

namespace App\Notifications\Forms;

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Notifications\Messages\MailMessage;
use Illuminate\Notifications\Notification;

class MobileEditorEmail extends Notification implements ShouldQueue
{
use Queueable;

protected $slug;

/**
* Create a new notification instance.
*
* @param string $slug
* @return void
*/
public function __construct($slug)
{
$this->slug = $slug;
}

/**
* Get the notification's delivery channels.
*
* @param mixed $notifiable
* @return array
*/
public function via($notifiable)
{
return ['mail'];
}

/**
* Get the mail representation of the notification.
*
* @param mixed $notifiable
* @return \Illuminate\Notifications\Messages\MailMessage
*/
public function toMail($notifiable)
{
return (new MailMessage())
->subject('Continue editing your form on desktop')
->line('We noticed you\'re editing a form on smaller screen.')
->line('For the best form building experience, we recommend using a desktop computer.')
->line('Ready to create something amazing? Click below to continue editing. 💻')
->action(__('Continue Editing'), front_url('forms/' . $this->slug . '/edit'));
}
}
1 change: 1 addition & 0 deletions api/routes/api.php
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@
Route::post('/{id}/workspace/{workspace_id}', [FormController::class, 'updateWorkspace'])->name('workspace.update');
Route::put('/{id}', [FormController::class, 'update'])->name('update');
Route::delete('/{id}', [FormController::class, 'destroy'])->name('destroy');
Route::get('/{id}/mobile-editor-email', [FormController::class, 'mobileEditorEmail'])->name('mobile-editor-email');

Route::get('/{id}/submissions', [FormSubmissionController::class, 'submissions'])->name('submissions');
Route::put('/{id}/submissions/{submission_id}', [FormSubmissionController::class, 'update'])->name('submissions.update')->middleware([ResolveFormMiddleware::class]);
Expand Down
11 changes: 11 additions & 0 deletions client/components/open/forms/components/FormEditor.vue
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ import { captureException } from "@sentry/core"
import FormSettings from './form-components/FormSettings.vue'
import FormEditorErrorHandler from '~/components/open/forms/components/FormEditorErrorHandler.vue'
import { setFormDefaults } from '~/composables/forms/initForm.js'
import { breakpointsTailwind, useBreakpoints } from '@vueuse/core'

export default {
name: "FormEditor",
Expand Down Expand Up @@ -145,6 +146,16 @@ export default {
emits: ['mounted', 'on-save', 'openRegister', 'go-back', 'save-form'],

setup() {
// Check if the editor is visible on smaller screens then send an email
const breakpoints = useBreakpoints(breakpointsTailwind)
const isVisible = ref(breakpoints.smaller("md"))
watch(isVisible, (newValue) => {
if (newValue) {
opnFetch('/open/forms/' + form.value.id + '/mobile-editor-email')
}
})
Comment on lines +149 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Several improvements needed for mobile detection logic.

The current implementation has potential issues:

  1. Missing debounce/throttle could lead to multiple API calls during browser resizing
  2. No error handling for the API call
  3. Potential null reference when accessing form.value.id
  4. Watch handler might trigger unnecessarily on component mount
  5. No state tracking to prevent duplicate notifications

Consider this improved implementation:

 setup() {
+    const hasNotified = ref(false)
     const breakpoints = useBreakpoints(breakpointsTailwind)
     const isVisible = ref(breakpoints.smaller("md"))
-    watch(isVisible, (newValue) => {
-      if (newValue) {
-        opnFetch('/open/forms/' + form.value.id + '/mobile-editor-email')
+    watch(isVisible, (newValue, oldValue) => {
+      // Skip initial trigger and check if already notified
+      if (oldValue === undefined || hasNotified.value || !form.value?.id) return
+      
+      if (newValue) {
+        hasNotified.value = true
+        opnFetch('/open/forms/' + form.value.id + '/mobile-editor-email')
+          .catch(error => {
+            console.error('Failed to send mobile editor notification:', error)
+          })
       }
-    })
+    }, { flush: 'post' })

Additionally, consider using useDebouncedRef from @vueuse/core to prevent rapid changes during browser resizing:

-    const isVisible = ref(breakpoints.smaller("md"))
+    const isVisible = useDebouncedRef(breakpoints.smaller("md"), 300)

Committable suggestion skipped: line range outside the PR's diff.



const { user } = storeToRefs(useAuthStore())
const formsStore = useFormsStore()
const { content: form } = storeToRefs(useWorkingFormStore())
Expand Down
Loading