fix: Dedupe authors (#1018)

closes #752
chanzuckerberg · Aug 12, 2024 · bdbc034 · bdbc034
1 parent 6e43b85
commit bdbc034
Show file tree

Hide file tree

Showing 2 changed files with 132 additions and 99 deletions.
diff --git a/frontend/packages/data-portal/app/components/AuthorList/AuthorList.test.tsx b/frontend/packages/data-portal/app/components/AuthorList/AuthorList.test.tsx
@@ -18,108 +18,140 @@ const AUTHOR_MAP = Object.fromEntries(
   DEFAULT_AUTHORS.map((author) => [author.name, author]),
 )
 
-it('should render authors', () => {
-  render(<AuthorList authors={DEFAULT_AUTHORS} />)
+describe('non-compact', () => {
+  it('should render authors', () => {
+    render(<AuthorList authors={DEFAULT_AUTHORS} />)
 
-  DEFAULT_AUTHORS.forEach((author) =>
-    expect(screen.getByText(author.name)).toBeInTheDocument(),
-  )
-})
+    DEFAULT_AUTHORS.forEach((author) =>
+      expect(screen.getByText(author.name)).toBeInTheDocument(),
+    )
+  })
 
-it('should sort primary authors', () => {
-  render(<AuthorList authors={DEFAULT_AUTHORS} />)
-  const authorNode = screen.getByRole('paragraph')
-  const authors = (authorNode.textContent ?? '').split(', ')
+  it('should sort primary authors', () => {
+    render(<AuthorList authors={DEFAULT_AUTHORS} />)
+    const authors = findAuthorStrings()
 
-  expect(AUTHOR_MAP[authors[0]].primary_author_status).toBe(true)
-  expect(AUTHOR_MAP[authors[1]].primary_author_status).toBe(true)
-})
+    expect(AUTHOR_MAP[authors[0]].primary_author_status).toBe(true)
+    expect(AUTHOR_MAP[authors[1]].primary_author_status).toBe(true)
+  })
 
-it('should sort other authors', () => {
-  render(<AuthorList authors={DEFAULT_AUTHORS} />)
-  const authorNode = screen.getByRole('paragraph')
-  const authors = (authorNode.textContent ?? '').split(', ')
-  const otherAuthors = authors.slice(2, -2)
+  it('should sort other authors', () => {
+    render(<AuthorList authors={DEFAULT_AUTHORS} />)
+    const authors = findAuthorStrings()
+    const otherAuthors = authors.slice(2, -2)
 
-  otherAuthors.forEach((author) => {
-    expect(AUTHOR_MAP[author].primary_author_status).toBeUndefined()
-    expect(AUTHOR_MAP[author].corresponding_author_status).toBeUndefined()
+    otherAuthors.forEach((author) => {
+      expect(AUTHOR_MAP[author].primary_author_status).toBeUndefined()
+      expect(AUTHOR_MAP[author].corresponding_author_status).toBeUndefined()
+    })
   })
-})
 
-it('should sort corresponding authors', () => {
-  render(<AuthorList authors={DEFAULT_AUTHORS} />)
-  const authorNode = screen.getByRole('paragraph')
-  const authors = (authorNode.textContent ?? '').split(', ')
-
-  expect(AUTHOR_MAP[authors.at(-1) ?? ''].corresponding_author_status).toBe(
-    true,
-  )
-  expect(AUTHOR_MAP[authors.at(-2) ?? ''].corresponding_author_status).toBe(
-    true,
-  )
-})
+  it('should sort corresponding authors', () => {
+    render(<AuthorList authors={DEFAULT_AUTHORS} />)
+    const authors = findAuthorStrings()
 
-it('should render author links', () => {
-  const authors = DEFAULT_AUTHORS.map((author, idx) => ({
-    ...author,
-    orcid: `0000-0000-0000-000${idx}`,
-  }))
+    expect(AUTHOR_MAP[authors.at(-1) ?? ''].corresponding_author_status).toBe(
+      true,
+    )
+    expect(AUTHOR_MAP[authors.at(-2) ?? ''].corresponding_author_status).toBe(
+      true,
+    )
+  })
 
-  render(<AuthorList authors={authors} AuthorLinkComponent={MockAuthorLink} />)
+  it('should render author links', () => {
+    const authors = DEFAULT_AUTHORS.map((author, idx) => ({
+      ...author,
+      orcid: `0000-0000-0000-000${idx}`,
+    }))
+
+    render(
+      <AuthorList authors={authors} AuthorLinkComponent={MockAuthorLink} />,
+    )
+
+    authors.forEach((author) =>
+      expect(
+        screen.getByRole('link', { name: `${author.name}` }),
+      ).toBeInTheDocument(),
+    )
+  })
 
-  authors.forEach((author) =>
-    expect(
-      screen.getByRole('link', { name: `${author.name}` }),
-    ).toBeInTheDocument(),
-  )
+  it('should not display any author more than once', () => {
+    render(
+      <AuthorList
+        authors={[
+          {
+            name: 'One',
+            primary_author_status: true,
+            corresponding_author_status: true,
+          },
+          {
+            name: 'Two',
+          },
+        ]}
+      />,
+    )
+
+    const authors = findAuthorStrings()
+
+    expect(authors.length).toBe(2)
+    expect(authors[0]).toBe('One')
+    expect(authors[1]).toBe('Two')
+  })
 })
 
-it('should not render author links when compact', () => {
-  const authors = DEFAULT_AUTHORS.map((author, idx) => ({
-    ...author,
-    orcid: `0000-0000-0000-000${idx}`,
-  }))
-
-  render(
-    <AuthorList
-      authors={authors}
-      AuthorLinkComponent={MockAuthorLink}
-      compact
-    />,
-  )
-
-  expect(screen.queryByRole('link')).not.toBeInTheDocument()
-})
+describe('compact', () => {
+  it('should not render author links when compact', () => {
+    const authors = DEFAULT_AUTHORS.map((author, idx) => ({
+      ...author,
+      orcid: `0000-0000-0000-000${idx}`,
+    }))
+
+    render(
+      <AuthorList
+        authors={authors}
+        AuthorLinkComponent={MockAuthorLink}
+        compact
+      />,
+    )
+
+    expect(screen.queryByRole('link')).not.toBeInTheDocument()
+  })
 
-it('should not render other authors when compact', () => {
-  render(<AuthorList authors={DEFAULT_AUTHORS} compact />)
-  const authorNode = screen.getByRole('paragraph')
-  const authors = (authorNode.textContent ?? '').split(', ')
-  const otherAuthors = authors.slice(2, -2)
+  it('should not render other authors when compact', () => {
+    render(<AuthorList authors={DEFAULT_AUTHORS} compact />)
+    const authorNode = screen.getByRole('paragraph')
+    const authors = (authorNode.textContent ?? '').split(', ')
+    const otherAuthors = authors.slice(2, -2)
 
-  otherAuthors.forEach((author) =>
-    expect(screen.queryByText(author)).not.toBeInTheDocument(),
-  )
-})
+    otherAuthors.forEach((author) =>
+      expect(screen.queryByText(author)).not.toBeInTheDocument(),
+    )
+  })
 
-it('should render comma if compact and has corresponding authors', () => {
-  render(<AuthorList authors={DEFAULT_AUTHORS} compact />)
-  expect(screen.getByText((text) => text.includes('... ,'))).toBeInTheDocument()
-})
+  it('should render comma if compact and has corresponding authors', () => {
+    render(<AuthorList authors={DEFAULT_AUTHORS} compact />)
+    expect(
+      screen.getByText((text) => text.includes('... ,')),
+    ).toBeInTheDocument()
+  })
 
-it('should not render comma for others if compact and no corresponding authors', () => {
-  render(
-    <AuthorList
-      authors={DEFAULT_AUTHORS.filter(
-        (author) => !author.corresponding_author_status,
-      )}
-      compact
-    />,
-  )
-
-  expect(screen.getByText((text) => text.includes('...'))).toBeInTheDocument()
-  expect(
-    screen.queryByText((text) => text.includes('... ,')),
-  ).not.toBeInTheDocument()
+  it('should not render comma for others if compact and no corresponding authors', () => {
+    render(
+      <AuthorList
+        authors={DEFAULT_AUTHORS.filter(
+          (author) => !author.corresponding_author_status,
+        )}
+        compact
+      />,
+    )
+
+    expect(screen.getByText((text) => text.includes('...'))).toBeInTheDocument()
+    expect(
+      screen.queryByText((text) => text.includes('... ,')),
+    ).not.toBeInTheDocument()
+  })
 })
+
+function findAuthorStrings(): string[] {
+  return (screen.getByRole('paragraph').textContent ?? '').split(', ')
+}
diff --git a/frontend/packages/data-portal/app/components/AuthorList/AuthorList.tsx b/frontend/packages/data-portal/app/components/AuthorList/AuthorList.tsx
@@ -28,17 +28,18 @@ export function AuthorList({
   large?: boolean
   subtle?: boolean
 }) {
-  // TODO: make the below grouping more efficient and/or use GraphQL ordering
-  const authorsPrimary = authors.filter(
-    (author) => author.primary_author_status,
-  )
-  const authorsCorresponding = authors.filter(
-    (author) => author.corresponding_author_status,
-  )
-  const authorsOther = authors.filter(
-    (author) =>
-      !(author.primary_author_status || author.corresponding_author_status),
-  )
+  const authorsPrimary = []
+  const authorsOther = []
+  const authorsCorresponding = []
+  for (const author of authors) {
+    if (author.primary_author_status) {
+      authorsPrimary.push(author)
+    } else if (author.corresponding_author_status) {
+      authorsCorresponding.push(author)
+    } else {
+      authorsOther.push(author)
+    }
+  }
 
   const otherCollapsed = useMemo<string | null>(() => {
     const ellipsis = '...'