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

add folderId to environment variables #233

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

juliawegmayr
Copy link
Contributor

@juliawegmayr juliawegmayr commented Nov 28, 2024

Description

Add folderId to environment variables to allow overriding default value 1.

Changeset

[x] I have verified if my change requires a changeset

Related tasks and documents

https://vivid-planet.atlassian.net/browse/COM-1280

@juliawegmayr juliawegmayr marked this pull request as ready for review November 28, 2024 08:17
@auto-assign auto-assign bot requested a review from raphaelblum November 28, 2024 08:17
.env Outdated
@@ -85,3 +85,4 @@ NEXT_PUBLIC_CAMPAIGN_IS_PREVIEW=false
REDIRECT_URL_FOR_IMPORT=$SITE_URL
BREVO_ALLOWED_REDIRECT_URL=http://${DEV_DOMAIN:-localhost}${WORKAROUND_DOTENV_ISSUE}:${SITE_PORT}
CAMPAIGNS_FRONTEND_URL=http://${DEV_DOMAIN:-localhost}${WORKAROUND_DOTENV_ISSUE}:${SITE_PORT} # doesn't work, TODO: add actual mailing frontend
BREVO_FOLDER_ID=
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would either set it to 1 here or not set it at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -15,6 +15,7 @@ export interface BrevoModuleConfig {
};
allowedRedirectUrl: string;
redirectUrlForImport: string;
folderId: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a breaking change.
I would make it optional and set it to default 1 in the package, not in the demo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try {
const contactList = {
name: title,
folderId: 1, // folderId is required, folder #1 is created by default
folderId: folderId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
folderId: folderId,
folderId,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

sonarcloud bot commented Dec 13, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants