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 11 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
Expand Up @@ -4,7 +4,13 @@ import { UseMutationResult, useQuery } from "@tanstack/react-query"
import React, { useEffect, useState } from "react"
import { useTranslation } from "react-i18next"

import { fetchBackgroundQuestionsAndAnswers } from "../../services/backend"
import {
fetchBackgroundQuestionsAndAnswers,
fetchCourseById,
updateMarketingConsent,
} from "../../services/backend"

import SelectMarketingConsentForm from "./SelectMarketingConsentForm"

import { CourseInstance, NewCourseBackgroundQuestionAnswer } from "@/shared-module/common/bindings"
import Button from "@/shared-module/common/components/Button"
Expand Down Expand Up @@ -40,24 +46,41 @@ interface SelectCourseInstanceFormProps {
>
initialSelectedInstanceId?: string
dialogLanguage: string
selectedLangCourseId: string
}

const SelectCourseInstanceForm: React.FC<
React.PropsWithChildren<SelectCourseInstanceFormProps>
> = ({ courseInstances, submitMutation, initialSelectedInstanceId, dialogLanguage }) => {
> = ({
courseInstances,
submitMutation,
initialSelectedInstanceId,
dialogLanguage,
selectedLangCourseId,
}) => {
const { t } = useTranslation("course-material", { lng: dialogLanguage })
const [selectedInstanceId, setSelectedInstanceId] = useState(
figureOutInitialValue(courseInstances, initialSelectedInstanceId),
)
const [additionalQuestionAnswers, setAdditionalQuestionAnswers] = useState<
NewCourseBackgroundQuestionAnswer[]
>([])

const [isMarketingConsentChecked, setIsMarketingConsentChecked] = useState(false)
const [isEmailSubscriptionConsentChecked, setIsEmailSubscriptionConsentChecked] = useState(false)

const additionalQuestionsQuery = useQuery({
queryKey: ["additional-questions", selectedInstanceId],
queryFn: () => fetchBackgroundQuestionsAndAnswers(assertNotNullOrUndefined(selectedInstanceId)),
enabled: selectedInstanceId !== undefined,
})

const getCourse = useQuery({
queryKey: ["courses", selectedLangCourseId],
queryFn: () => fetchCourseById(selectedLangCourseId as NonNullable<string>),
enabled: selectedLangCourseId !== null,
})
Comment on lines +78 to +82
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

Correct the 'enabled' condition in the getCourse query

The enabled condition checks selectedLangCourseId !== null, but since selectedLangCourseId is of type string, it cannot be null. Consider removing the enabled condition or updating it to check for undefined if selectedLangCourseId can be undefined.


useEffect(() => {
if (!additionalQuestionsQuery.data) {
return
Expand Down Expand Up @@ -100,6 +123,14 @@ const SelectCourseInstanceForm: React.FC<
backgroundQuestionAnswers: additionalQuestionAnswers,
})
}
if (getCourse.isSuccess) {
await updateMarketingConsent(
getCourse.data.id,
getCourse.data.course_language_group_id,
isEmailSubscriptionConsentChecked,
isMarketingConsentChecked,
)
}
Comment on lines +126 to +133
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

Add error handling for getCourse query failure

Currently, updateMarketingConsent is called only if getCourse.isSuccess is true, but there's no error handling for when getCourse fails. Consider handling the failure case to inform the user or retry the fetch operation.

}

const additionalQuestions = additionalQuestionsQuery.data?.background_questions
Expand Down Expand Up @@ -191,12 +222,25 @@ const SelectCourseInstanceForm: React.FC<
{additionalQuestionsQuery.error && (
<ErrorBanner variant="readOnly" error={additionalQuestionsQuery.error} />
)}
{getCourse.data?.ask_marketing_consent && (
<div>
<SelectMarketingConsentForm
courseId={selectedLangCourseId}
onEmailSubscriptionConsentChange={setIsEmailSubscriptionConsentChecked}
onMarketingConsentChange={setIsMarketingConsentChecked}
/>
</div>
)}
<div>
<Button
size="medium"
variant="primary"
onClick={enrollOnCourse}
disabled={!selectedInstanceId || additionalQuestionsQuery.isPending}
disabled={
!selectedInstanceId ||
additionalQuestionsQuery.isPending ||
(getCourse.data?.ask_marketing_consent && !isEmailSubscriptionConsentChecked)
}
Comment on lines +239 to +243
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

Ensure both consents are required before enabling the 'Continue' button

The disabled condition for the 'Continue' button only checks isEmailSubscriptionConsentChecked. If both marketing consent and email subscription consent are required, consider updating the condition to also check isMarketingConsentChecked to prevent users from proceeding without providing both consents.

data-testid="select-course-instance-continue-button"
>
{t("continue")}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { useQuery } from "@tanstack/react-query"
import { t } from "i18next"
import React, { useEffect, useState } from "react"

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

interface SelectMarketingConsentFormProps {
courseId: string
onEmailSubscriptionConsentChange: (isChecked: boolean) => void
onMarketingConsentChange: (isChecked: boolean) => void
}

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

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

const handleEmailSubscriptionConsentChange = (event: React.ChangeEvent<HTMLInputElement>) => {
const isChecked = event.target.checked
onEmailSubscriptionConsentChange(isChecked)
setEmailSubscriptionConsent(isChecked)
}

const handleMarketingConsentChange = (event: React.ChangeEvent<HTMLInputElement>) => {
const isChecked = event.target.checked
onMarketingConsentChange(isChecked)
setMarketingConsent(isChecked)
}

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)
const emailSub =
fetchInitialMarketingConsent.data.email_subscription_in_mailchimp === "subscribed"
setEmailSubscriptionConsent(emailSub)
}
}, [fetchInitialMarketingConsent.data, fetchInitialMarketingConsent.isSuccess])

return (
<>
<CheckBox
label={t("marketing-consent-checkbox-text")}
type="checkbox"
checked={marketingConsent}
onChange={handleMarketingConsentChange}
></CheckBox>
<CheckBox
label={t("marketing-consent-privacy-policy-checkbox-text")}
type="checkbox"
checked={emailSubscriptionConsent}
onChange={handleEmailSubscriptionConsentChange}
></CheckBox>
</>
)
}

export default SelectMarketingConsentForm
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@ 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,
fetchUserMarketingConsent,
postSaveCourseSettings,
} from "../../services/backend"
import SelectCourseLanguage from "../SelectCourseLanguage"
import SelectCourseInstanceForm from "../forms/SelectCourseInstanceForm"

Expand All @@ -23,6 +28,7 @@ import LoginStateContext from "@/shared-module/common/contexts/LoginStateContext
import useToastMutation from "@/shared-module/common/hooks/useToastMutation"
import { baseTheme, fontWeights, primaryFont, typography } from "@/shared-module/common/styles"
import { LANGUAGE_COOKIE_KEY } from "@/shared-module/common/utils/constants"
import { assertNotNullOrUndefined } from "@/shared-module/common/utils/nullability"
import withErrorBoundary from "@/shared-module/common/utils/withErrorBoundary"

export interface CourseSettingsModalProps {
Expand Down Expand Up @@ -71,6 +77,18 @@ const CourseSettingsModal: React.FC<React.PropsWithChildren<CourseSettingsModalP
})
sortInstances()

const askMarketingConsent = useQuery({
queryKey: ["courses", selectedLangCourseId],
queryFn: () => fetchCourseById(selectedLangCourseId as NonNullable<string>),
enabled: selectedLangCourseId !== null,
}).data?.ask_marketing_consent

Comment on lines +80 to +85
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

Handle loading and error states for askMarketingConsent

The askMarketingConsent value is directly accessed without handling loading or error states from the getCourse query. This might lead to unexpected behavior if the data is not yet available or if an error occurs. Consider checking getCourse.isLoading and getCourse.isError before using getCourse.data.

const checkUserMarketingConsent = useQuery({
queryKey: ["marketing-consent", selectedLangCourseId],
queryFn: () => fetchUserMarketingConsent(assertNotNullOrUndefined(selectedLangCourseId)),
enabled: selectedLangCourseId !== undefined,
}).data?.email_subscription_in_mailchimp

useEffect(() => {
getCourseInstances.refetch()
sortInstances()
Expand All @@ -88,8 +106,12 @@ const CourseSettingsModal: React.FC<React.PropsWithChildren<CourseSettingsModalP
const shouldChooseInstance =
pageState.state === "ready" && pageState.instance === null && pageState.settings === null

setOpen((signedIn && shouldChooseInstance) || (signedIn && manualOpen))
}, [loginState, pageState, manualOpen])
setOpen(
(signedIn && shouldChooseInstance) ||
(signedIn && manualOpen) ||
(signedIn && askMarketingConsent === true && checkUserMarketingConsent === "unsubscribed"),
)
}, [loginState, pageState, manualOpen, askMarketingConsent, checkUserMarketingConsent])
Comment on lines +109 to +114
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

Improve the condition for opening the modal

The condition for setting open combines several states but may not account for loading or error states of the queries. Consider adding checks for getCourse.isSuccess and checkUserMarketingConsent to ensure the modal only opens when the data is available and reliable.


const languageChanged = savedOrDefaultLangCourseId !== selectedLangCourseId

Expand Down Expand Up @@ -182,9 +204,11 @@ const CourseSettingsModal: React.FC<React.PropsWithChildren<CourseSettingsModalP
pageState.settings?.current_course_instance_id ?? pageState.instance?.id
}
dialogLanguage={dialogLanguage}
selectedLangCourseId={selectedLangCourseId}
/>
)}
</div>

{languageChanged && (
<div
className={css`
Expand Down
29 changes: 29 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,30 @@ 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,
emailSubscription: boolean,
marketingConsent: boolean,
): Promise<string> => {
const res = await courseMaterialClient.post(
`/courses/${courseId}/user-marketing-consent`,
{
course_language_groups_id: courseLanguageGroupsId,
email_subscription: emailSubscription,
marketing_consent: marketingConsent,
},
{
responseType: "json",
},
)
return validateResponse(res, isString)
}

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.';
Loading
Loading