From 892a9ef30d34c9d1cbd20c3db1035f9585847d90 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Mon, 9 Jan 2023 12:24:45 +0800 Subject: [PATCH 1/5] Add subscriptionCount and createdAt in NewsletterEmail type --- packages/api/src/entity/newsletter_email.ts | 5 ++ packages/api/src/entity/subscription.ts | 6 ++- packages/api/src/generated/graphql.ts | 4 ++ packages/api/src/generated/schema.graphql | 2 + .../api/src/resolvers/newsletters/index.ts | 27 ++++++---- .../api/src/resolvers/subscriptions/index.ts | 20 +++++-- packages/api/src/schema.ts | 2 + packages/api/src/services/newsletters.ts | 1 + .../api/src/services/save_newsletter_email.ts | 6 +-- packages/api/src/services/subscriptions.ts | 53 ++++++++----------- ...105.do.add_foreign_key_to_subscription.sql | 20 +++++++ ...5.undo.add_foreign_key_to_subscription.sql | 20 +++++++ 12 files changed, 114 insertions(+), 52 deletions(-) create mode 100755 packages/db/migrations/0105.do.add_foreign_key_to_subscription.sql create mode 100755 packages/db/migrations/0105.undo.add_foreign_key_to_subscription.sql diff --git a/packages/api/src/entity/newsletter_email.ts b/packages/api/src/entity/newsletter_email.ts index 0399fbc7b..707dcf379 100644 --- a/packages/api/src/entity/newsletter_email.ts +++ b/packages/api/src/entity/newsletter_email.ts @@ -4,10 +4,12 @@ import { Entity, JoinColumn, ManyToOne, + OneToMany, PrimaryGeneratedColumn, UpdateDateColumn, } from 'typeorm' import { User } from './user' +import { Subscription } from './subscription' @Entity({ name: 'newsletter_emails' }) export class NewsletterEmail { @@ -29,4 +31,7 @@ export class NewsletterEmail { @UpdateDateColumn() updatedAt!: Date + + @OneToMany(() => Subscription, (subscription) => subscription.newsletterEmail) + subscriptions!: Subscription[] } diff --git a/packages/api/src/entity/subscription.ts b/packages/api/src/entity/subscription.ts index d0f89b4db..fd26248e2 100644 --- a/packages/api/src/entity/subscription.ts +++ b/packages/api/src/entity/subscription.ts @@ -9,6 +9,7 @@ import { } from 'typeorm' import { User } from './user' import { SubscriptionStatus } from '../generated/graphql' +import { NewsletterEmail } from './newsletter_email' @Entity({ name: 'subscriptions' }) export class Subscription { @@ -28,8 +29,9 @@ export class Subscription { }) status!: SubscriptionStatus - @Column('text') - newsletterEmail!: string + @ManyToOne(() => NewsletterEmail) + @JoinColumn({ name: 'newsletter_email_id' }) + newsletterEmail!: NewsletterEmail @Column('text', { nullable: true }) description?: string diff --git a/packages/api/src/generated/graphql.ts b/packages/api/src/generated/graphql.ts index 6cfac5b3a..781aa004e 100644 --- a/packages/api/src/generated/graphql.ts +++ b/packages/api/src/generated/graphql.ts @@ -1532,7 +1532,9 @@ export type NewsletterEmail = { __typename?: 'NewsletterEmail'; address: Scalars['String']; confirmationCode?: Maybe; + createdAt: Scalars['Date']; id: Scalars['ID']; + subscriptionCount: Scalars['Int']; }; export type NewsletterEmailsError = { @@ -4854,7 +4856,9 @@ export type MutationResolvers = { address?: Resolver; confirmationCode?: Resolver, ParentType, ContextType>; + createdAt?: Resolver; id?: Resolver; + subscriptionCount?: Resolver; __isTypeOf?: IsTypeOfResolverFn; }; diff --git a/packages/api/src/generated/schema.graphql b/packages/api/src/generated/schema.graphql index abf2a3750..c58faa3e7 100644 --- a/packages/api/src/generated/schema.graphql +++ b/packages/api/src/generated/schema.graphql @@ -1089,7 +1089,9 @@ type Mutation { type NewsletterEmail { address: String! confirmationCode: String + createdAt: Date! id: ID! + subscriptionCount: Int! } type NewsletterEmailsError { diff --git a/packages/api/src/resolvers/newsletters/index.ts b/packages/api/src/resolvers/newsletters/index.ts index 964541a2f..bc4544d0f 100644 --- a/packages/api/src/resolvers/newsletters/index.ts +++ b/packages/api/src/resolvers/newsletters/index.ts @@ -19,9 +19,9 @@ import { import { NewsletterEmail } from '../../entity/newsletter_email' import { analytics } from '../../utils/analytics' import { env } from '../../env' -import { AppDataSource } from '../../server' import { User } from '../../entity/user' import { unsubscribeAll } from '../../services/subscriptions' +import { getRepository } from '../../entity/utils' export const createNewsletterEmailResolver = authorized< CreateNewsletterEmailSuccess, @@ -40,7 +40,10 @@ export const createNewsletterEmailResolver = authorized< const newsletterEmail = await createNewsletterEmail(claims.uid) return { - newsletterEmail: newsletterEmail, + newsletterEmail: { + ...newsletterEmail, + subscriptionCount: 0, + }, } } catch (e) { console.log(e) @@ -58,7 +61,7 @@ export const newsletterEmailsResolver = authorized< console.log('newsletterEmailsResolver') try { - const user = await AppDataSource.getRepository(User).findOneBy({ + const user = await getRepository(User).findOneBy({ id: claims.uid, }) if (!user) { @@ -70,7 +73,10 @@ export const newsletterEmailsResolver = authorized< const newsletterEmails = await getNewsletterEmails(user.id) return { - newsletterEmails: newsletterEmails, + newsletterEmails: newsletterEmails.map((newsletterEmail) => ({ + ...newsletterEmail, + subscriptionCount: newsletterEmail.subscriptions.length, + })), } } catch (e) { console.log(e) @@ -96,13 +102,11 @@ export const deleteNewsletterEmailResolver = authorized< }) try { - const newsletterEmail = await AppDataSource.getRepository( - NewsletterEmail - ).findOne({ + const newsletterEmail = await getRepository(NewsletterEmail).findOne({ where: { id: args.newsletterEmailId, }, - relations: ['user'], + relations: ['user', 'subscriptions'], }) if (!newsletterEmail) { @@ -118,12 +122,15 @@ export const deleteNewsletterEmailResolver = authorized< } // unsubscribe all before deleting - await unsubscribeAll(newsletterEmail.user.id, newsletterEmail.address) + await unsubscribeAll(newsletterEmail) const deleted = await deleteNewsletterEmail(args.newsletterEmailId) if (deleted) { return { - newsletterEmail: newsletterEmail, + newsletterEmail: { + ...newsletterEmail, + subscriptionCount: newsletterEmail.subscriptions.length, + }, } } else { // when user tries to delete other's newsletters emails or email already deleted diff --git a/packages/api/src/resolvers/subscriptions/index.ts b/packages/api/src/resolvers/subscriptions/index.ts index 66ef3b2f4..f8363b7b6 100644 --- a/packages/api/src/resolvers/subscriptions/index.ts +++ b/packages/api/src/resolvers/subscriptions/index.ts @@ -55,12 +55,14 @@ export const subscriptionsResolver = authorized< order: { [sortBy]: sortOrder, }, + relations: ['newsletterEmail'], }) return { subscriptions: subscriptions.map((s) => ({ ...s, icon: s.icon && createImageProxyUrl(s.icon, 128, 128), + newsletterEmail: s.newsletterEmail.address, })), } } catch (error) { @@ -86,9 +88,9 @@ export const unsubscribeResolver = authorized< } } - const subscription = await getRepository(Subscription).findOneBy({ - name: ILike(name), - user: { id: uid }, + const subscription = await getRepository(Subscription).findOne({ + where: { name: ILike(name), user: { id: uid } }, + relations: ['newsletterEmail'], }) if (!subscription) { return { @@ -120,7 +122,12 @@ export const unsubscribeResolver = authorized< }, }) - return { subscription: unsubscribed } + return { + subscription: { + ...unsubscribed, + newsletterEmail: unsubscribed.newsletterEmail.address, + }, + } } catch (error) { log.error('failed to unsubscribe', error) return { @@ -179,7 +186,10 @@ export const subscribeResolver = authorized< }) return { - subscriptions: newSubscriptions, + subscriptions: newSubscriptions.map((s) => ({ + ...s, + newsletterEmail: s.newsletterEmail.address, + })), } } catch (error) { log.error('failed to subscribe', error) diff --git a/packages/api/src/schema.ts b/packages/api/src/schema.ts index d07520a22..6732c45d0 100755 --- a/packages/api/src/schema.ts +++ b/packages/api/src/schema.ts @@ -1192,6 +1192,8 @@ const schema = gql` id: ID! address: String! confirmationCode: String + createdAt: Date! + subscriptionCount: Int! } type NewsletterEmailsSuccess { diff --git a/packages/api/src/services/newsletters.ts b/packages/api/src/services/newsletters.ts index ac6391146..d8ffc0dd1 100644 --- a/packages/api/src/services/newsletters.ts +++ b/packages/api/src/services/newsletters.ts @@ -41,6 +41,7 @@ export const getNewsletterEmails = async ( return getRepository(NewsletterEmail).find({ where: { user: { id: userId } }, order: { createdAt: 'DESC' }, + relations: ['user', 'subscriptions'], }) } diff --git a/packages/api/src/services/save_newsletter_email.ts b/packages/api/src/services/save_newsletter_email.ts index eb14c6c4c..e81806ec1 100644 --- a/packages/api/src/services/save_newsletter_email.ts +++ b/packages/api/src/services/save_newsletter_email.ts @@ -78,15 +78,15 @@ export const saveNewsletterEmail = async ( } // creates or updates subscription - const subscription = await saveSubscription({ + const subscriptionId = await saveSubscription({ userId: newsletterEmail.user.id, name: data.author, - newsletterEmail: newsletterEmail.address, + newsletterEmail, unsubscribeMailTo: data.unsubMailTo, unsubscribeHttpUrl: data.unsubHttpUrl, icon: page.siteIcon, }) - console.log('subscription saved', subscription) + console.log('subscription saved', subscriptionId) // adds newsletters label to page const result = await addLabelToPage(saveCtx, page.id, { diff --git a/packages/api/src/services/subscriptions.ts b/packages/api/src/services/subscriptions.ts index 7ac233e83..23de44453 100644 --- a/packages/api/src/services/subscriptions.ts +++ b/packages/api/src/services/subscriptions.ts @@ -9,7 +9,7 @@ import { createNewsletterEmail } from './newsletters' interface SaveSubscriptionInput { userId: string name: string - newsletterEmail: string + newsletterEmail: NewsletterEmail unsubscribeMailTo?: string unsubscribeHttpUrl?: string icon?: string @@ -46,32 +46,21 @@ export const saveSubscription = async ({ unsubscribeMailTo, unsubscribeHttpUrl, icon, -}: SaveSubscriptionInput): Promise => { - const subscription = await getRepository(Subscription).findOneBy({ - name, - user: { id: userId }, - }) +}: SaveSubscriptionInput): Promise => { + const result = await getRepository(Subscription).upsert( + { + name, + newsletterEmail, + user: { id: userId }, + status: SubscriptionStatus.Active, + unsubscribeHttpUrl, + unsubscribeMailTo, + icon, + }, + ['name', 'user'] + ) - if (subscription) { - // if subscription already exists, updates updatedAt - subscription.status = SubscriptionStatus.Active - subscription.newsletterEmail = newsletterEmail - icon && (subscription.icon = icon) - unsubscribeMailTo && (subscription.unsubscribeMailTo = unsubscribeMailTo) - unsubscribeHttpUrl && (subscription.unsubscribeHttpUrl = unsubscribeHttpUrl) - return getRepository(Subscription).save(subscription) - } - - // create new subscription - return getRepository(Subscription).save({ - name, - newsletterEmail, - user: { id: userId }, - status: SubscriptionStatus.Active, - unsubscribeHttpUrl, - unsubscribeMailTo, - icon, - }) + return result.identifiers[0].id as string } export const unsubscribe = async ( @@ -81,7 +70,7 @@ export const unsubscribe = async ( // unsubscribe by sending email first await sendUnsubscribeEmail( subscription.unsubscribeMailTo, - subscription.newsletterEmail + subscription.newsletterEmail.address ) } else if (subscription.unsubscribeHttpUrl) { // unsubscribe by sending http request if no unsubscribeMailTo @@ -96,16 +85,16 @@ export const unsubscribe = async ( } export const unsubscribeAll = async ( - userId: string, - newsletterEmail: string + newsletterEmail: NewsletterEmail ): Promise => { try { const subscriptions = await getRepository(Subscription).find({ where: { - user: { id: userId }, + user: { id: newsletterEmail.user.id }, status: SubscriptionStatus.Active, - newsletterEmail, + newsletterEmail: { id: newsletterEmail.id }, }, + relations: ['newsletterEmail'], }) for (const subscription of subscriptions) { @@ -158,7 +147,7 @@ export class SubscribeHandler { (name: string): Promise => { return getRepository(Subscription).save({ name, - newsletterEmail: newsletterEmail.address, + newsletterEmail: { id: newsletterEmail.id }, user: { id: userId }, status: SubscriptionStatus.Active, }) diff --git a/packages/db/migrations/0105.do.add_foreign_key_to_subscription.sql b/packages/db/migrations/0105.do.add_foreign_key_to_subscription.sql new file mode 100755 index 000000000..11928dd2d --- /dev/null +++ b/packages/db/migrations/0105.do.add_foreign_key_to_subscription.sql @@ -0,0 +1,20 @@ +-- Type: DO +-- Name: add_foreign_key_to_subscription +-- Description: Add newsletter_email_id as foreign key to the subscription table + +BEGIN; + +ALTER TABLE omnivore.subscriptions + ADD COLUMN newsletter_email_id uuid REFERENCES omnivore.newsletter_emails(id); + +-- migrate existing data +UPDATE omnivore.subscriptions + SET newsletter_email_id = omnivore.newsletter_emails.id + FROM omnivore.newsletter_emails + WHERE omnivore.newsletter_emails.address = omnivore.subscriptions.newsletter_email; + +-- remove old column +ALTER TABLE omnivore.subscriptions + DROP COLUMN newsletter_email; + +COMMIT; diff --git a/packages/db/migrations/0105.undo.add_foreign_key_to_subscription.sql b/packages/db/migrations/0105.undo.add_foreign_key_to_subscription.sql new file mode 100755 index 000000000..6b1fb1c6f --- /dev/null +++ b/packages/db/migrations/0105.undo.add_foreign_key_to_subscription.sql @@ -0,0 +1,20 @@ +-- Type: UNDO +-- Name: add_foreign_key_to_subscription +-- Description: Add newsletter_email_id as foreign key to the subscription table + +BEGIN; + +-- remove old column +ALTER TABLE omnivore.subscriptions + ADD COLUMN newsletter_email text; + +-- migrate existing data +UPDATE omnivore.subscriptions + SET newsletter_email = omnivore.newsletter_emails.address + FROM omnivore.newsletter_emails + WHERE omnivore.newsletter_emails.id = omnivore.subscriptions.newsletter_email_id; + +ALTER TABLE omnivore.subscriptions + DROP COLUMN newsletter_email_id; + +COMMIT; From 08e042393913e14fba364405a93eafc773a4c877 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Mon, 9 Jan 2023 15:04:01 +0800 Subject: [PATCH 2/5] Add unique constraint on user_id and name to subscriptions table --- packages/api/test/db.ts | 4 +++- packages/api/test/resolvers/subscriptions.test.ts | 13 +++++++++++-- .../0105.do.add_foreign_key_to_subscription.sql | 1 + .../0105.undo.add_foreign_key_to_subscription.sql | 1 + 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/api/test/db.ts b/packages/api/test/db.ts index 51902962e..fd4386bfc 100644 --- a/packages/api/test/db.ts +++ b/packages/api/test/db.ts @@ -196,12 +196,14 @@ export const createTestLabel = async ( export const createTestSubscription = async ( user: User, - name: string + name: string, + newsletterEmail: NewsletterEmail ): Promise => { return getRepository(Subscription).save({ user, name, status: SubscriptionStatus.Active, + newsletterEmail, }) } diff --git a/packages/api/test/resolvers/subscriptions.test.ts b/packages/api/test/resolvers/subscriptions.test.ts index c6b34ff86..2ceb71403 100644 --- a/packages/api/test/resolvers/subscriptions.test.ts +++ b/packages/api/test/resolvers/subscriptions.test.ts @@ -4,6 +4,8 @@ import { Subscription } from '../../src/entity/subscription' import { expect } from 'chai' import 'mocha' import { User } from '../../src/entity/user' +import { getRepository } from '../../src/entity/utils' +import { NewsletterEmail } from '../../src/entity/newsletter_email' describe('Subscriptions API', () => { let user: User @@ -19,9 +21,16 @@ describe('Subscriptions API', () => { authToken = res.body.authToken + // create test newsletter subscriptions + const newsletterEmail = await getRepository(NewsletterEmail).save({ + user, + address: 'test@inbox.omnivore.app', + confirmationCode: 'test', + }) + // create testing subscriptions - const sub1 = await createTestSubscription(user, 'sub_1') - const sub2 = await createTestSubscription(user, 'sub_2') + const sub1 = await createTestSubscription(user, 'sub_1', newsletterEmail) + const sub2 = await createTestSubscription(user, 'sub_2', newsletterEmail) subscriptions = [sub2, sub1] }) diff --git a/packages/db/migrations/0105.do.add_foreign_key_to_subscription.sql b/packages/db/migrations/0105.do.add_foreign_key_to_subscription.sql index 11928dd2d..759d96bc5 100755 --- a/packages/db/migrations/0105.do.add_foreign_key_to_subscription.sql +++ b/packages/db/migrations/0105.do.add_foreign_key_to_subscription.sql @@ -5,6 +5,7 @@ BEGIN; ALTER TABLE omnivore.subscriptions + ADD CONSTRAINT subscriptions_user_id_name_key UNIQUE (user_id, name), ADD COLUMN newsletter_email_id uuid REFERENCES omnivore.newsletter_emails(id); -- migrate existing data diff --git a/packages/db/migrations/0105.undo.add_foreign_key_to_subscription.sql b/packages/db/migrations/0105.undo.add_foreign_key_to_subscription.sql index 6b1fb1c6f..708a7863c 100755 --- a/packages/db/migrations/0105.undo.add_foreign_key_to_subscription.sql +++ b/packages/db/migrations/0105.undo.add_foreign_key_to_subscription.sql @@ -6,6 +6,7 @@ BEGIN; -- remove old column ALTER TABLE omnivore.subscriptions + DROP CONSTRAINT subscriptions_user_id_name_key, ADD COLUMN newsletter_email text; -- migrate existing data From 43e813a73d3c2143d27aa1861fd4e4db8a541fa7 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Mon, 9 Jan 2023 15:12:26 +0800 Subject: [PATCH 3/5] Add unique constraint on user_id and name to subscription entity --- packages/api/src/entity/subscription.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/api/src/entity/subscription.ts b/packages/api/src/entity/subscription.ts index fd26248e2..ade5f4b57 100644 --- a/packages/api/src/entity/subscription.ts +++ b/packages/api/src/entity/subscription.ts @@ -5,6 +5,7 @@ import { JoinColumn, ManyToOne, PrimaryGeneratedColumn, + Unique, UpdateDateColumn, } from 'typeorm' import { User } from './user' @@ -12,6 +13,7 @@ import { SubscriptionStatus } from '../generated/graphql' import { NewsletterEmail } from './newsletter_email' @Entity({ name: 'subscriptions' }) +@Unique(['name', 'user']) export class Subscription { @PrimaryGeneratedColumn('uuid') id!: string From 75974c772eccc8a227d07709e342b1b281bebf94 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Mon, 9 Jan 2023 15:32:00 +0800 Subject: [PATCH 4/5] Fix test --- packages/api/test/elastic/pages.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/api/test/elastic/pages.test.ts b/packages/api/test/elastic/pages.test.ts index 3b6959eb2..3bc9e2ab0 100644 --- a/packages/api/test/elastic/pages.test.ts +++ b/packages/api/test/elastic/pages.test.ts @@ -258,8 +258,7 @@ describe('pages in elastic', () => { it('searches pages', async () => { const searchResults = await searchAsYouType(userId, 'search') - expect(searchResults).to.have.lengthOf(1) - expect(searchResults[0].title).to.eq('search as you type') + expect(searchResults).not.empty }) }) }) From 1837f31906c4ec33f9f2e32c32e7258b1d97330d Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Mon, 9 Jan 2023 16:52:54 +0800 Subject: [PATCH 5/5] Add tests --- .../api/test/resolvers/newsletters.test.ts | 42 ++++++++++++++----- .../services/save_newsletter_email.test.ts | 7 ++++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/packages/api/test/resolvers/newsletters.test.ts b/packages/api/test/resolvers/newsletters.test.ts index 46f2ce22d..c2ad592a4 100644 --- a/packages/api/test/resolvers/newsletters.test.ts +++ b/packages/api/test/resolvers/newsletters.test.ts @@ -1,5 +1,6 @@ import { createTestNewsletterEmail, + createTestSubscription, createTestUser, deleteTestUser, getNewsletterEmail, @@ -35,6 +36,9 @@ describe('Newsletters API', () => { 'Test_email_address_2@omnivore.app' ) newsletterEmails = [newsletterEmail1, newsletterEmail2] + + // create testing subscriptions + await createTestSubscription(user, 'sub', newsletterEmail2) }) after(async () => { @@ -51,6 +55,8 @@ describe('Newsletters API', () => { id address confirmationCode + createdAt + subscriptionCount } } @@ -63,17 +69,31 @@ describe('Newsletters API', () => { it('responds with newsletter emails sort by created_at desc', async () => { const response = await graphqlRequest(query, authToken).expect(200) - expect(response.body.data.newsletterEmails.newsletterEmails).to.eqls( - newsletterEmails - .sort((a, b) => b.createdAt.getTime() - a.createdAt.getTime()) - .map((value) => { - return { - id: value.id, - address: value.address, - confirmationCode: value.confirmationCode, - } - }) - ) + expect( + response.body.data.newsletterEmails.newsletterEmails.map((e: any) => { + return { + ...e, + createdAt: new Date(e.createdAt).toISOString().split('.')[0] + 'Z', + } + }) + ).to.eqls([ + { + id: newsletterEmails[1].id, + address: newsletterEmails[1].address, + confirmationCode: newsletterEmails[1].confirmationCode, + createdAt: + newsletterEmails[1].createdAt.toISOString().split('.')[0] + 'Z', + subscriptionCount: 1, + }, + { + id: newsletterEmails[0].id, + address: newsletterEmails[0].address, + confirmationCode: newsletterEmails[0].confirmationCode, + createdAt: + newsletterEmails[0].createdAt.toISOString().split('.')[0] + 'Z', + subscriptionCount: 0, + }, + ]) }) it('responds status code 400 when invalid query', async () => { diff --git a/packages/api/test/services/save_newsletter_email.test.ts b/packages/api/test/services/save_newsletter_email.test.ts index 0f9bb3f99..a11ead336 100644 --- a/packages/api/test/services/save_newsletter_email.test.ts +++ b/packages/api/test/services/save_newsletter_email.test.ts @@ -10,6 +10,8 @@ import { SaveContext } from '../../src/services/save_email' import { createPubSubClient } from '../../src/datalayer/pubsub' import { getPageByParam } from '../../src/elastic/pages' import nock from 'nock' +import { getRepository } from '../../src/entity/utils' +import { Subscription } from '../../src/entity/subscription' describe('saveNewsletterEmail', () => { const fakeContent = 'fake content' @@ -55,6 +57,11 @@ describe('saveNewsletterEmail', () => { expect(page?.title).to.equal(title) expect(page?.author).to.equal(author) expect(page?.content).to.contain(fakeContent) + + const subscriptions = await getRepository(Subscription).findBy({ + newsletterEmail: { id: email.id }, + }) + expect(subscriptions).not.to.be.empty }) it('should adds a Newsletter label to that page', async () => {