Feature/107 Unsubscribe from emails #115
Conversation
WalkthroughThis update enhances the application's flexibility and usability by introducing a subscription management system, allowing users to easily control their email preferences. Key changes include modifications to database schemas for tracking subscription status, improved ESLint configurations for testing, and comprehensive unit tests to ensure reliability. Additionally, the user interface has been updated to provide clear options for managing subscriptions, enriching the overall user experience. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant EmailModule
participant Database
User->>SettingsPage: View email settings
SettingsPage->>Database: Fetch subscription status
Database-->>SettingsPage: Return status (subscribed/unsubscribed)
SettingsPage->>User: Display current status
User->>SettingsPage: Toggle subscription
SettingsPage->>Database: Update subscription status
Database-->>SettingsPage: Confirm update
SettingsPage->>User: Show updated status
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- .eslintrc.cjs (1 hunks)
- .vscode/settings.json (1 hunks)
- README.md (1 hunks)
- database_migration.sql (2 hunks)
- src/DatabaseDefinitions.ts (3 hunks)
- src/lib/emails/welcome_email_html.svelte (3 hunks)
- src/lib/emails/welcome_email_text.svelte (2 hunks)
- src/lib/mailer.test.ts (1 hunks)
- src/lib/mailer.ts (3 hunks)
- src/routes/(admin)/account/(menu)/settings/+page.svelte (1 hunks)
- src/routes/(admin)/account/(menu)/settings/change_email_subscription/+page.svelte (1 hunks)
- src/routes/(admin)/account/api/+page.server.ts (2 hunks)
- src/routes/(admin)/account/api/page.server.test.ts (1 hunks)
- supabase/migrations/20240730010101_initial.sql (1 hunks)
- supabase/migrations/20240731051052_add_unsubscribed_to_profiles.sql (1 hunks)
- tsconfig.json (1 hunks)
- vite.config.ts (1 hunks)
Additional comments not posted (37)
supabase/migrations/20240731051052_add_unsubscribed_to_profiles.sql (1)
1-2: Verify the impact of the new column addition.Ensure that adding the
unsubscribedcolumn to theprofilestable does not conflict with existing data and that the default value is set correctly.Verification successful
The addition of the
unsubscribedcolumn to theprofilestable does not conflict with existing data and the default value is set correctly.
- The
unsubscribedcolumn is referenced and used correctly in multiple files, including SQL migration files, TypeScript definitions, server-side scripts, and tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the new column addition to the `profiles` table. # Test: Check if the column `unsubscribed` exists in the `profiles` table and verify its default value. echo "SELECT column_name, column_default FROM information_schema.columns WHERE table_name = 'profiles' AND column_name = 'unsubscribed';" | psqlLength of output: 230
Script:
#!/bin/bash # Search for references to the `profiles` table in the codebase to verify the handling of the `unsubscribed` column. rg 'profiles' -A 5 # Search for references to the `unsubscribed` column in the codebase to verify its usage. rg 'unsubscribed' -A 5Length of output: 22200
vite.config.ts (1)
8-8: LGTM! But verify the impact on existing tests.The addition of
globals: truesimplifies the test setup by allowing the use of global test functions. Ensure that this change does not introduce any unintended side effects in existing tests..vscode/settings.json (1)
3-12: LGTM! But verify the impact of the new formatter and linting settings.The updates specify distinct formatters for Svelte, TypeScript, and JavaScript files, and include TypeScript in the ESLint validation array. Ensure that these changes are correctly applied and do not conflict with existing settings.
Verification successful
The current jq queries in the script do not correctly target the nested formatter settings for Svelte, TypeScript, and JavaScript. Let's adjust the queries to ensure they correctly access these nested configurations.
Let's verify the actual content of the .vscode/settings.json file to ensure it contains the expected configurations for formatters.
The formatter and linting settings are correctly applied.
The .vscode/settings.json file contains the correct configurations for the formatters for Svelte, TypeScript, and JavaScript, as well as the ESLint validation settings.
- Svelte formatter:
svelte.svelte-vscode- TypeScript formatter:
esbenp.prettier-vscode- JavaScript formatter:
esbenp.prettier-vscode- ESLint validation:
["javascript", "javascriptreact", "typescript", "svelte"]Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the new formatter and linting settings. # Test: Check if the formatters are correctly applied to Svelte, TypeScript, and JavaScript files. echo "Checking formatters..." echo "Svelte formatter: $(jq -r '."editor.defaultFormatter"' .vscode/settings.json)" echo "TypeScript formatter: $(jq -r '."[typescript].editor.defaultFormatter"' .vscode/settings.json)" echo "JavaScript formatter: $(jq -r '."[javascript].editor.defaultFormatter"' .vscode/settings.json)" # Test: Check if ESLint validation is applied to TypeScript files. echo "Checking ESLint validation..." echo "ESLint validation: $(jq -r '."eslint.validate"' .vscode/settings.json)"Length of output: 745
Script:
#!/bin/bash # Description: Verify the impact of the new formatter and linting settings. # Test: Check if the formatters are correctly applied to Svelte, TypeScript, and JavaScript files. echo "Checking formatters..." echo "Svelte formatter: $(jq -r '."[svelte]".editor.defaultFormatter' .vscode/settings.json)" echo "TypeScript formatter: $(jq -r '."[typescript]".editor.defaultFormatter' .vscode/settings.json)" echo "JavaScript formatter: $(jq -r '."[javascript]".editor.defaultFormatter' .vscode/settings.json)" # Test: Check if ESLint validation is applied to TypeScript files. echo "Checking ESLint validation..." echo "ESLint validation: $(jq -r '."eslint.validate"' .vscode/settings.json)"Length of output: 754
Script:
#!/bin/bash # Description: Output the entire content of the .vscode/settings.json file for inspection. echo "Content of .vscode/settings.json:" cat .vscode/settings.jsonLength of output: 455
src/lib/emails/welcome_email_text.svelte (2)
2-2: Import statement forWebsiteBaseUrllooks good.The import statement correctly brings in
WebsiteBaseUrlfrom the configuration file.
18-18: Unsubscribe link added successfully.The unsubscribe link is correctly formatted and uses
WebsiteBaseUrlto direct users to the subscription settings page.tsconfig.json (1)
12-12: Addition of Vitest global types is appropriate.The
"types": ["vitest/globals"]property allows TypeScript to recognize global types provided by Vitest, enhancing the testing experience.src/routes/(admin)/account/(menu)/settings/change_email_subscription/+page.svelte (5)
2-2: Import statement forSettingsModulelooks good.The import statement correctly brings in
SettingsModulefrom the specified path.
3-6: Data handling is appropriate.The data handling correctly extracts
profileandunsubscribedstatus from the provided data.
8-10: Page title is set correctly.The page title is appropriately set to "Change Email Subscription".
12-14: Header displays the correct message based on subscription status.The header correctly displays "Re-subscribe to Emails" or "Unsubscribe from Emails" based on the
unsubscribedstatus.
16-26: SettingsModule component is configured correctly.The
SettingsModulecomponent is correctly configured with appropriate properties for handling email subscription changes..eslintrc.cjs (1)
29-35: LGTM! Verify the ESLint configuration.The new configuration block for test files is reasonable and aligns with common practices in testing where strict type checking can be relaxed.
However, ensure that the ESLint configuration is correctly applied to the intended test files.
Verification successful
To verify the ESLint configuration for test files, we will inspect the
.eslintrc.cjsfile to ensure that the configuration block disabling the@typescript-eslint/no-explicit-anyrule for test files is correctly applied.
To verify the ESLint configuration for test files, we will inspect the entire content of the
.eslintrc.cjsfile to ensure that the configuration block disabling the@typescript-eslint/no-explicit-anyrule for test files is correctly applied.
LGTM! ESLint configuration verified.
The configuration block for test files is correctly applied in the
.eslintrc.cjsfile, disabling the@typescript-eslint/no-explicit-anyrule for**/*.test.tsand**/*.spec.tsfiles.
- The configuration block is present and correctly formatted in the
.eslintrc.cjsfile.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ESLint configuration for test files. # Test: Run ESLint on test files. Expect: No warnings for `any` type. eslint "**/*.test.ts" "**/*.spec.ts"Length of output: 112
Script:
#!/bin/bash # Search for the configuration block in the .eslintrc.cjs file # Display the relevant lines from the .eslintrc.cjs file grep -A 5 '"**/*.test.ts", "**/*.spec.ts"' .eslintrc.cjsLength of output: 58
Script:
#!/bin/bash # Display the entire content of the .eslintrc.cjs file for manual inspection cat .eslintrc.cjsLength of output: 1366
src/routes/(admin)/account/(menu)/settings/+page.svelte (1)
55-66: LGTM! Verify the functionality of the new component.The new
SettingsModulecomponent for managing email subscriptions is well-integrated and enhances the user experience.However, ensure that the component functions correctly and dynamically updates based on the user's subscription status.
Verification successful
Let's gather more information about the
SettingsModulecomponent and its logic to verify its functionality.
LGTM! Verify the functionality of the new component.
The new
SettingsModulecomponent for managing email subscriptions is well-integrated and enhances the user experience. The component dynamically updates based on the user's subscription status, as confirmed by the gathered context.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the functionality of the new `SettingsModule` component. # Test: Run the application and check the settings page. Expect: The component displays the correct subscription status and the edit button works as expected. # Note: This is a manual verification step. echo "Please manually verify the functionality of the new 'SettingsModule' component on the settings page."Length of output: 216
Script:
#!/bin/bash # Find the definition of the SettingsModule component fd 'SettingsModule.svelte' --exec cat {} # Find all references to the SettingsModule component to understand how it is used rg 'SettingsModule' -A 5 # Check for the logic that handles the subscription status and the edit button rg 'subscriptionStatus' -A 5 rg 'unsubscribed' -A 5Length of output: 24586
database_migration.sql (1)
8-9: LGTM! Verify the database migration.The new
unsubscribedcolumn in theprofilestable is a straightforward addition that aligns with the PR objectives.However, ensure that the database migration is applied correctly and the column behaves as expected.
Verification successful
Verified: The
unsubscribedcolumn is correctly added to theprofilestable with the expected default value offalse.
CREATE TABLE profilesstatement includesunsubscribed boolean NOT NULL DEFAULT false.ALTER TABLE profilesstatement includesADD COLUMN IF NOT EXISTS unsubscribed boolean NOT NULL DEFAULT false.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the database migration for the `unsubscribed` column. # Test: Apply the migration and check the `profiles` table. Expect: The `unsubscribed` column is added with the correct default value. # Note: This is a manual verification step. echo "Please manually verify the database migration for the 'unsubscribed' column in the 'profiles' table."Length of output: 222
Script:
#!/bin/bash # Description: Check the schema of the `profiles` table to verify the `unsubscribed` column. # Locate the SQL files and extract table creation or alteration statements for the `profiles` table. fd -e sql -x cat {} + | grep -A 10 -i "CREATE TABLE profiles" | grep -i "unsubscribed" fd -e sql -x cat {} + | grep -A 10 -i "ALTER TABLE profiles" | grep -i "unsubscribed"Length of output: 496
supabase/migrations/20240730010101_initial.sql (5)
1-9: LGTM! Table creation and RLS policies forprofilesare well-defined.The table includes necessary columns and references the
auth.userstable. The RLS policies ensure that users can only view and modify their own profiles, which is a good security practice.
24-31: LGTM! Table creation and RLS policies forstripe_customersare well-defined.The table includes necessary columns for Stripe customer information and references the
auth.userstable. The RLS policies ensure that access to the table is restricted, which is appropriate for sensitive information.
34-45: LGTM! Table creation and RLS policies forcontact_requestsare well-defined.The table includes necessary columns for contact form submissions. The RLS policies ensure that access to the table is restricted, which is appropriate for sensitive information.
48-60: LGTM! Trigger for creating profile entries on user sign-up is well-defined.The trigger
on_auth_user_createdensures that a profile entry is created for each new user, which is necessary for maintaining user data consistency.
62-72: LGTM! Storage setup and access policies for avatars are well-defined.The storage setup includes creating a bucket for avatars and defining access policies. The policies ensure that avatar images are publicly accessible and that anyone can upload an avatar.
src/DatabaseDefinitions.ts (1)
53-53: LGTM! The addition of theunsubscribedproperty is well-defined.The
unsubscribedproperty is necessary for managing user email preferences and is correctly added to theSelect,Insert, andUpdatesections of theprofilestable.Also applies to: 62-62, 71-71
src/lib/mailer.test.ts (6)
1-3: LGTM! Mocking of dependencies is well-defined.The dependencies are correctly mocked to isolate the tests for the
mailermodule.
9-22: LGTM! Mocking of Supabase client is well-defined.The Supabase client is correctly mocked to isolate the tests for the
mailermodule.
24-39: LGTM! Setup for tests is well-defined.The setup for the tests includes clearing mocks and setting environment variables, which is necessary for ensuring test isolation and consistency.
41-66: LGTM! Test for sending a welcome email is well-defined.The test correctly verifies that a welcome email is sent to the user and includes appropriate assertions.
68-99: LGTM! Test for handling unsubscribed users is well-defined.The test correctly verifies that an email is not sent to unsubscribed users and includes appropriate assertions.
102-119: LGTM! Test for sending a templated email is well-defined.The test correctly verifies that a templated email is sent and includes appropriate assertions.
src/lib/mailer.ts (2)
6-6: Good practice: Type safety improvement.Adding the type parameter
<Database>to thecreateClientcall improves type safety and ensures that database interactions conform to the expected schema.
77-92: Ensure error handling for profile fetch.The logic for checking the subscription status is correct. However, ensure that the error handling for the profile fetch operation is comprehensive.
Verify that the error logging provides sufficient information for debugging.
src/routes/(admin)/account/api/page.server.test.ts (4)
31-44: LGTM! Test case for no session redirection.The test case correctly mocks the dependencies and verifies that the user is redirected to the login page when there is no session.
46-71: LGTM! Test case for toggling subscription status from false to true.The test case correctly mocks the dependencies and verifies that the subscription status is toggled from false to true.
73-98: LGTM! Test case for toggling subscription status from true to false.The test case correctly mocks the dependencies and verifies that the subscription status is toggled from true to false.
100-122: LGTM! Test case for handling update operation failure.The test case correctly mocks the dependencies and verifies that the function returns a failure response when the update operation fails.
src/routes/(admin)/account/api/+page.server.ts (2)
5-31: LGTM!toggleEmailSubscriptionaction.The action correctly handles session validation, fetches the current subscription status, updates the status, and handles errors appropriately.
285-285: LGTM! Modification toupdateEmailaction.The modification correctly initializes the
unsubscribedfield during the update process.src/lib/emails/welcome_email_html.svelte (2)
2-2: LGTM! Import statement is correct.The import statement for
WebsiteBaseUrlis syntactically correct and aligns with the intended functionality.
263-275: LGTM! Unsubscribe link is correctly implemented.The "Unsubscribe" link uses the
WebsiteBaseUrlvariable and directs users to the email subscription settings page. The styling is consistent with the rest of the email template.README.md (1)
196-203: LGTM! Improved clarity in setup instructions.The expanded setup instructions provide clear and detailed steps for both new and existing Supabase projects, enhancing the usability of the setup process.
|
Thanks! I’m a bit slammed but will take a look when I have some time! |
| successBody={unsubscribed | ||
| ? "You have been re-subscribed to emails" | ||
| : "You have been unsubscribed from all emails"} | ||
| formTarget="/account/api?/toggleEmailSubscription" |
There was a problem hiding this comment.
nitpick: I'd make this explicitly pass the true/false value instead of toggle. Technically UI could be stale and button does the opposite of what it says. Not likely in practice/P2. Will merge anyways, but if we're making other changes might be nice to add.
There was a problem hiding this comment.
Yep, I would prefer an explicit true/false request as well, but went with path of least resistance here :D
I can I work on this if you are not doing it, and will submit as a separate PR?
| : "Unsubscribe from all emails"} | ||
| successBody={unsubscribed | ||
| ? "You have been re-subscribed to emails" | ||
| : "You have been unsubscribed from all emails"} |
There was a problem hiding this comment.
Nitpick: I'd remove word "all". Things like "reset password" will still be sent.
There was a problem hiding this comment.
I did this one while making some visual tweaks to settings. Those green buttons were bothering me. Other one still outstanding/optional.
|
Really fantastic PR. Thanks @evgenyneu. Merging as is. Few nitpick things like wording, but those are optional and can come after. I might quickly do them now. |
Added ability for users to unsubscribe from emails (#107).
/account/settings/change_email_subscriptionpage{WebsiteBaseUrl}/account/settings/change_email_subscription:text:
Issue to be fixed: If user it not logged in they will not be redirected to
/account/settings/change_email_subscriptionafter login, this could be implemented later?Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests