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

Mailchimp #1332

Merged
merged 19 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 8 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { useQuery } from "@tanstack/react-query"
import React, { useEffect, useState } from "react"

import { fetchUserMarketingConsent, updateMarketingConsent } from "@/services/backend"
import CheckBox from "@/shared-module/common/components/InputFields/CheckBox"
import useToastMutation from "@/shared-module/common/hooks/useToastMutation"
import { assertNotNullOrUndefined } from "@/shared-module/common/utils/nullability"

interface SelectMarketingConsentFormProps {
courseId: string
courseLanguageGroupsId: string
}

const SelectMarketingConsentForm: React.FC<SelectMarketingConsentFormProps> = ({
courseId,
courseLanguageGroupsId,
}) => {
const [marketingConsent, setMarketingConsent] = useState(false)

const fetchInitialMarketingConsent = useQuery({
queryKey: ["marketing-consent", courseId],
queryFn: () => fetchUserMarketingConsent(assertNotNullOrUndefined(courseId)),
enabled: courseId !== undefined,
})

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

I think this you useQuery hook and after that useEffect if needed

if (fetchInitialMarketingConsent.isSuccess) {
setMarketingConsent(fetchInitialMarketingConsent.data.consent)
}
}, [fetchInitialMarketingConsent.data, fetchInitialMarketingConsent.isSuccess])

const handleMarketingConsentChangeMutation = useToastMutation(
async () => {
try {
await updateMarketingConsent(courseId, courseLanguageGroupsId, marketingConsent)
} catch (error) {
setMarketingConsent(!marketingConsent)
}
return null
},
{ notify: false },
)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve mutation error handling and user feedback

  1. Consider enabling error notifications for users
  2. Add loading state during mutation
  3. Implement proper error recovery for optimistic updates
 const handleMarketingConsentChangeMutation = useToastMutation(
   async () => {
+    const previousConsent = marketingConsent
     try {
       await updateMarketingConsent(courseId, courseLanguageGroupsId, marketingConsent)
     } catch (error) {
-      setMarketingConsent(!marketingConsent)
+      setMarketingConsent(previousConsent)
+      throw new Error('Failed to update marketing consent')
     }
     return null
   },
-  { notify: false },
+  {
+    notify: true,
+    errorMessage: 'Failed to update marketing consent. Please try again.',
+  },
 )

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


return (
<>
<CheckBox
// eslint-disable-next-line i18next/no-literal-string
label="I consent to receive marketing messages for this course."
type="checkbox"
checked={marketingConsent}
onChange={() => {
setMarketingConsent(!marketingConsent)
handleMarketingConsentChangeMutation.mutate()
}}
></CheckBox>
</>
)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve accessibility and internationalization

  1. The label text should be internationalized instead of being hardcoded
  2. Add loading state handling in the UI
  3. Disable the checkbox during loading states
 return (
   <>
     <CheckBox
-      // eslint-disable-next-line i18next/no-literal-string
-      label="I consent to receive marketing messages for this course."
+      label={t('marketing.consent.label')}
       type="checkbox"
       checked={marketingConsent}
+      disabled={isLoading || handleMarketingConsentChangeMutation.isLoading}
       onChange={() => {
         setMarketingConsent(!marketingConsent)
         handleMarketingConsentChangeMutation.mutate()
       }}
     ></CheckBox>
+    {(isLoading || handleMarketingConsentChangeMutation.isLoading) && (
+      <LoadingSpinner size="small" />
+    )}
   </>
 )

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

}

export default SelectMarketingConsentForm
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,14 @@ import React, { useContext, useEffect, useId, useState } from "react"
import { useTranslation } from "react-i18next"

import PageContext from "../../contexts/PageContext"
import { fetchCourseInstances, postSaveCourseSettings } from "../../services/backend"
import {
fetchCourseById,
fetchCourseInstances,
postSaveCourseSettings,
} from "../../services/backend"
import SelectCourseLanguage from "../SelectCourseLanguage"
import SelectCourseInstanceForm from "../forms/SelectCourseInstanceForm"
import SelectMarketingConsentForm from "../forms/SelectMarketingConsentForm"

import {
getLanguageName,
Expand Down Expand Up @@ -71,6 +76,12 @@ const CourseSettingsModal: React.FC<React.PropsWithChildren<CourseSettingsModalP
})
sortInstances()

const getCourse = useQuery({
queryKey: ["courses", selectedLangCourseId],
queryFn: () => fetchCourseById(selectedLangCourseId as NonNullable<string>),
enabled: selectedLangCourseId !== null && open && pageState.state === "ready",
})

useEffect(() => {
getCourseInstances.refetch()
sortInstances()
Expand Down Expand Up @@ -185,6 +196,18 @@ const CourseSettingsModal: React.FC<React.PropsWithChildren<CourseSettingsModalP
/>
)}
</div>
<div
className={css`
padding: 1rem 3rem;
`}
>
{getCourse.data?.ask_marketing_consent && (
<SelectMarketingConsentForm
courseId={selectedLangCourseId}
courseLanguageGroupsId={getCourse.data?.course_language_group_id}
/>
)}
</div>
{languageChanged && (
<div
className={css`
Expand Down
27 changes: 27 additions & 0 deletions services/course-material/src/services/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import {
UserCourseInstanceChapterProgress,
UserCourseInstanceProgress,
UserCourseSettings,
UserMarketingConsent,
UserModuleCompletionStatus,
} from "@/shared-module/common/bindings"
import {
Expand Down Expand Up @@ -86,6 +87,7 @@ import {
isUserCourseInstanceChapterProgress,
isUserCourseInstanceProgress,
isUserCourseSettings,
isUserMarketingConsent,
isUserModuleCompletionStatus,
} from "@/shared-module/common/bindings.guard"
import {
Expand Down Expand Up @@ -743,3 +745,28 @@ export const claimCodeFromCodeGiveaway = async (id: string): Promise<string> =>
const response = await courseMaterialClient.post(`/code-giveaways/${id}/claim`)
return validateResponse(response, isString)
}

export const updateMarketingConsent = async (
courseId: string,
courseLanguageGroupsId: string,
consent: boolean,
): Promise<string> => {
const res = await courseMaterialClient.post(
`/courses/${courseId}/user-marketing-consent`,
{
course_language_groups_id: courseLanguageGroupsId,
consent,
},
{
responseType: "json",
},
)
return validateResponse(res, isString)
}
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

Fix the API endpoint path and consider improving the return type.

  1. The API endpoint path is missing a leading slash, which is inconsistent with other endpoints in this file.
  2. Consider using a more specific return type instead of string to better document the expected response.

Apply this diff to fix the path:

-    `/courses/${courseId}/user-marketing-consent`,
+    `/courses/${courseId}/user-marketing-consent`,

Consider creating a specific type for the response:

type MarketingConsentUpdateResponse = {
  status: string;
  // Add other relevant fields based on the actual API response
}


export const fetchUserMarketingConsent = async (
courseId: string,
): Promise<UserMarketingConsent> => {
const res = await courseMaterialClient.get(`/courses/${courseId}/fetch-user-marketing-consent`)
return validateResponse(res, isUserMarketingConsent)
}
4 changes: 4 additions & 0 deletions services/headless-lms/entrypoint/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ fn main() -> Result<()> {
name: "chatbot-syncer",
execute: Box::new(|| tokio_run(programs::chatbot_syncer::main())),
},
Program {
name: "mailchimp-syncer",
execute: Box::new(|| tokio_run(programs::mailchimp_syncer::main())),
},
Comment on lines +71 to +74
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

Documentation is missing for the mailchimp-syncer program

The search results confirm that while there are extensive code implementations related to marketing consent and the mailchimp-syncer program, there is no operator documentation describing:

  • Required environment variables and configuration
  • Expected behavior and execution frequency
  • Troubleshooting steps

The program appears to be a critical component that synchronizes marketing consents with Mailchimp, and proper documentation would help operators maintain and troubleshoot it effectively.

🔗 Analysis chain

Consider adding operator documentation for the new program.

Since this is a new program that handles marketing consent synchronization with Mailchimp, it would be helpful to document:

  • Required environment variables or configuration
  • Expected behavior and frequency of execution
  • Troubleshooting steps

Let's check if documentation exists:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for documentation about the mailchimp-syncer program
# Expected: Find documentation in README or docs

# Search for documentation files
fd -t f "README|DOCUMENTATION|\.md$"

# Search for mailchimp-syncer mentions in documentation
rg -i "mailchimp.?syncer|marketing.?consent" 

Length of output: 18085

];

let program_name = std::env::args().nth(1).unwrap_or_else(|| {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
ALTER TABLE courses DROP COLUMN ask_marketing_consent;
DROP TABLE user_marketing_consents;
DROP TABLE marketing_mailing_list_access_tokens;
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
ALTER TABLE courses
ADD COLUMN ask_marketing_consent BOOLEAN NOT NULL DEFAULT FALSE;

COMMENT ON COLUMN courses.ask_marketing_consent IS 'Whether this course asks the user for marketing consent.';

CREATE TABLE user_marketing_consents (
id UUID DEFAULT uuid_generate_v4() PRIMARY KEY,
course_id UUID NOT NULL REFERENCES courses(id),
course_language_group_id UUID NOT NULL REFERENCES course_language_groups(id),
user_id UUID NOT NULL REFERENCES users(id),
user_mailchimp_id VARCHAR(255),
consent BOOLEAN NOT NULL,
email_subscription_in_mailchimp VARCHAR(255),
created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(),
updated_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(),
deleted_at TIMESTAMP WITH TIME ZONE,
synced_to_mailchimp_at TIMESTAMP WITH TIME ZONE,
CONSTRAINT course_language_group_specific_marketing_user_uniqueness UNIQUE NULLS NOT DISTINCT(user_id, course_language_group_id)
);

CREATE TRIGGER set_timestamp BEFORE
UPDATE ON user_marketing_consents FOR EACH ROW EXECUTE PROCEDURE trigger_set_timestamp();

COMMENT ON TABLE user_marketing_consents IS 'This table is used to keep a record if a user has given a marketing consent to a course';
COMMENT ON COLUMN user_marketing_consents.id IS 'A unique, stable identifier for the record.';
COMMENT ON COLUMN user_marketing_consents.course_id IS 'Course that the user has access to.';
COMMENT ON COLUMN user_marketing_consents.course_language_group_id IS 'The course language group id that the mailing list is related to';
COMMENT ON COLUMN user_marketing_consents.user_id IS 'User who has the access to the course.';
COMMENT ON COLUMN user_marketing_consents.user_mailchimp_id IS 'Unique id for the user, provided by Mailchimp';
COMMENT ON COLUMN user_marketing_consents.consent IS 'Whether the user has given a marketing consent for a specific course.';
COMMENT ON COLUMN user_marketing_consents.email_subscription_in_mailchimp IS 'Tells the users email subscription status in Mailchimp';
COMMENT ON COLUMN user_marketing_consents.created_at IS 'Timestamp of when the record was created.';
COMMENT ON COLUMN user_marketing_consents.updated_at IS 'Timestamp when the record was last updated. The field is updated automatically by the set_timestamp trigger.';
COMMENT ON COLUMN user_marketing_consents.deleted_at IS 'Timestamp when the record was deleted. If null, the record is not deleted.';
COMMENT ON COLUMN user_marketing_consents.synced_to_mailchimp_at IS 'Timestamp when the record was synced to mailchimp. If null, the record has not been synced.';


CREATE TABLE marketing_mailing_list_access_tokens (
id UUID DEFAULT uuid_generate_v4() PRIMARY KEY,
course_id UUID NOT NULL REFERENCES courses(id),
course_language_group_id UUID NOT NULL REFERENCES course_language_groups(id),
server_prefix VARCHAR(255) NOT NULL,
access_token VARCHAR(255) NOT NULL,
mailchimp_mailing_list_id VARCHAR(255) NOT NULL,
nygrenh marked this conversation as resolved.
Show resolved Hide resolved
created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(),
updated_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(),
deleted_at TIMESTAMP WITH TIME ZONE
);

CREATE TRIGGER set_timestamp BEFORE
UPDATE ON marketing_mailing_list_access_tokens FOR EACH ROW EXECUTE PROCEDURE trigger_set_timestamp();

COMMENT ON TABLE marketing_mailing_list_access_tokens IS 'This table is used to keep a record of marketing mailing lists access tokens for each course language group';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.id IS 'A unique, stable identifier for the record.';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.course_id IS 'The course id that the the mailing list is related to';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.course_language_group_id IS 'The course language group id that the mailing list is related to';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.server_prefix IS 'This value is used to configure API requests to the correct Mailchimp server.';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.access_token IS 'Token used for access authentication.';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.mailchimp_mailing_list_id IS 'Id of the mailing list used for marketing in Mailchimp';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.created_at IS 'Timestamp when the record was created.';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.updated_at IS 'Timestamp when the record was last updated. The field is updated automatically by the set_timestamp trigger.';
COMMENT ON COLUMN marketing_mailing_list_access_tokens.deleted_at IS 'Timestamp when the record was deleted. If null, the record is not deleted.';

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading