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 17 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
54 changes: 54 additions & 0 deletions kubernetes/base/headless-lms/mailchimp-syncer.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: mailchimp-syncer
labels:
app: mailchimp-syncer
deploymentType: with-init-container
needs-db: "true"
spec:
replicas: 1
selector:
matchLabels:
app: mailchimp-syncer
template:
metadata:
annotations:
linkerd.io/inject: enabled
labels:
app: mailchimp-syncer
spec:
containers:
- name: mailchimp-syncer
image: headless-lms
command: ["bin/run", "mailchimp-syncer"]
resources:
requests:
memory: 100Mi
cpu: 20m
limits:
memory: 300Mi
cpu: 200m
envFrom:
- secretRef:
name: headless-lms-secrets
initContainers:
- name: headless-lms-wait-for-db
image: headless-lms
command:
- bash
- "-c"
- |
echo Waiting for postgres to be available
timeout 120 ./wait-for-db.sh
./wait-for-db-migrations.sh
Comment on lines +42 to +44
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

Review database wait timeout and add health checks.

Two potential improvements:

  1. The 120-second timeout for database readiness might be insufficient for large databases
  2. Missing liveness and readiness probes which are crucial for Kubernetes health monitoring

Add health check probes:

          livenessProbe:
            httpGet:
              path: /health
              port: 8080
            initialDelaySeconds: 30
            periodSeconds: 10
          readinessProbe:
            httpGet:
              path: /health
              port: 8080
            initialDelaySeconds: 5
            periodSeconds: 5

resources:
requests:
memory: 100Mi
cpu: 20m
limits:
memory: 300Mi
cpu: 200m
envFrom:
- secretRef:
name: headless-lms-secrets
1 change: 1 addition & 0 deletions kubernetes/base/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@ resources:
- headless-lms/peer-review-updater.yml
- headless-lms/sync-tmc-users.yml
- headless-lms/chatbot-syncer.yml
- headless-lms/mailchimp-syncer.yml
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 @@ -51,6 +51,7 @@ import {
UserCourseInstanceChapterProgress,
UserCourseInstanceProgress,
UserCourseSettings,
UserMarketingConsent,
UserModuleCompletionStatus,
} from "@/shared-module/common/bindings"
import {
Expand Down Expand Up @@ -90,6 +91,7 @@ import {
isUserCourseInstanceChapterProgress,
isUserCourseInstanceProgress,
isUserCourseSettings,
isUserMarketingConsent,
isUserModuleCompletionStatus,
} from "@/shared-module/common/bindings.guard"
import {
Expand Down Expand Up @@ -748,6 +750,33 @@ export const claimCodeFromCodeGiveaway = async (id: string): Promise<string> =>
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)
}

export const fetchPartnersBlock = async (courseId: string): Promise<PartnersBlock> => {
const response = await courseMaterialClient.get(`/courses/${courseId}/partners-block`)
return validateResponse(response, isPartnersBlock)
Expand Down
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;
Loading
Loading