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

Started as a email refactor to use a local mail server, ended up fixing several bugs related to content #2578

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

KaelenProctor
Copy link
Contributor

@KaelenProctor KaelenProctor commented Jan 26, 2025

Refactoring

  1. Consolidated the email transport/sending code into a utility file, with a simple sendEmail export
  2. That makes it easy to configure different email transports for production or local. Though I did learn how to configure my gmail with an app password in order to send emails from local. As well the code now gracefully warns if the required email config options are not set, rather than causing a server error

Example of local email server with mailpit + docker:
mailpit-local-email-testing

Bugs fixed

  1. The content creator application submission was not doing anything
  2. The admin routes related to content / contributors failed to send emails because the hydrated User properties don't have email on them
  3. Also fixed bugs with content model where put actions would replace the user/owner object with just their ID before writing to dynamo, with the side effect of that changing the input variable. Then afterwards in the route the object is different and doesn't match the type (looking to the future with typescript)
  4. Podcast and videos send for review used an incorrect review status
  5. Fixed the video content page not loading. It expected a list of videos but was getting one

Testing

Content creator application submission

Before:
Clicking the button did nothing
old-submit-content-creator-broken

After:
new-submit-content-creator-working

Failing to send content / contributor emails + model replacing user/owner

Before:
Can see the error as "no recipients" defined. The hydrated User object doesn't have email, have to explicitly get the user with sensitive data to have their email. Probably should adjust the email property on the User type for clarity.
old-failed-to-send-review-email

After:
new-content-published
new-content-reject-email
new-submit-content-creator-approve

Podcast and videos send for review used an incorrect review status

Before:
old-podcast-submit
old-video-submit
old-podcast-not-showing-for-review

After:
new-video-for-review

Video page not loading

Before:
old-video-broken

After:
new-video-working

Next steps

  1. Want to refactor the emails so that DOMAIN and http vs https are used as variables, so that local emails work well. Currently they all go to production so have to always copy and edit
  2. Some errors in the content reject email, where the content type isn't printed

No submit was occurring, nor was the input being added to the form data
that would have been submitted.
dynamo, which breaks the remaining controller logic.
approved or denied.

The standard User object does not contain the email property because it
is sensitive. In order to email the content owner we must explicitly get
their User with the sensitive details to have their email.
@KaelenProctor KaelenProctor force-pushed the feature/email-refactor-for-local branch from 8d41cd8 to 821a34b Compare January 26, 2025 13:39
@@ -104,17 +105,15 @@ account follow these steps:
2. Enter any label you wish (suggest CubeCobraLocalDev)
3. Set "reCAPTCHA type" to V2, with "I'm not a robot" tickbox enabled
4. Enter "localhost" as the domain
1. If you have setup your local CubeCobra to be accessible under a non-localhost domain (see [Running CubeCobra](#running-cubecobra)) then include that domain as well
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Auto-styling by IDE =(

@@ -112,7 +112,7 @@ const EditVideo: React.FC<EditVideoProps> = ({
</Col>
</Row>
<Text>
Write any supplmental text here. Cube Cobra uses a variation of markdown you can read about{' '}
Write any supplemental text here. Cube Cobra uses a variation of markdown you can read about{' '}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small typo

@@ -51,7 +56,7 @@ const ApplicationPage: React.FC<AdminDashboardPageProps> = ({ loginCallback = '/
value={info}
onChange={(e) => setInfo(e.target.value)}
/>
<Button color="primary" block outline>
<Button color="primary" block outline onClick={() => formRef.current?.submit()}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used other forms as a template. Button must submit form on click and the formData for CSRFForm must be updated with the state

username: string;
}

interface Video {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with real datatype

</Row>
</CardBody>
))}
<Video video={video} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use existing component which was already in use in the video preview

if (document[FIELDS.USER].id) {
document[FIELDS.USER] = document[FIELDS.USER].id;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as in content

transport: smtpTransport,
});
//Normal hydration of User does not contain email, thus we must fetch it in order to notify about their application
const owner = await User.getByIdWithSensitiveData(document.owner.id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hydrated user objects don't have email. In order to send the emails to the applications we must load their User with their sensitive data in order to have email

@@ -476,7 +476,7 @@ router.post('/submitpodcast', ensureContentCreator, async (req, res) => {
podcast.description = fields.description;
podcast.podcastLink = fields.url;
podcast.image = fields.image;
podcast.status = 'inReview';
podcast.status = Content.STATUS.IN_REVIEW;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using constants

pass: process.env.EMAIL_CONFIG_PASSWORD,
},
};
} else if (process.env.EMAIL_CONFIG_SMTP_HOST && process.env.EMAIL_CONFIG_SMTP_PORT) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is to use a local mail server here such as mailpit with docker. That is an easier setup than getting developers to create app passwords for their gmail (in large part because the google UI for it sucks)

const transport = getTransport();
if (!transport) {
// eslint-disable-next-line no-console -- Warn so clear why an email isn't being sent
console.warn('No email transport is configured, skipping sending');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the developer setup instructions don't include how to setup gmail for emails to start, the default behaviour meant sending emails just caused server errors. Rather than do that, might as well be graceful and not error.

@KaelenProctor KaelenProctor marked this pull request as ready for review January 26, 2025 13:50
@KaelenProctor
Copy link
Contributor Author

It might be worth checking in production if there are podcasts or videos in the wrong review status as typeStatusComp = 'v:inReview' or typeStatusComp = 'p:inReview', and then either deleting them as old or updating inReview to r

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.

1 participant