Do not link to documents for third-party annotations

- Refactor `ShowDocumentInfo`: don't link to documents for third-party
  annotations. This is a possibly-temporary measure to prevent linking
  to documents that are not directly accessible, e.g. in an LMS context.
- Remove unnecessary forwarding of `showDocumentInfo` through several
  components.
- Centralize some logic in `AnnotationHeader` and make `ShowDocumentInfo`
  a dumb component.

src/sidebar/components/Annotation/Annotation.js

@@ -26,7 +26,6 @@ import AnnotationReplyToggle from './AnnotationReplyToggle';
  * @prop {boolean} isReply
  * @prop {VoidFunction} onToggleReplies - Callback to expand/collapse reply threads
  * @prop {number} replyCount - Number of replies to this annotation's thread
- * @prop {boolean} showDocumentInfo - Should extended document info be rendered (e.g. in non-sidebar contexts)?
  * @prop {boolean} threadIsCollapsed - Is the thread to which this annotation belongs currently collapsed?
  * @prop {Object} annotationsService - Injected service
  */
@@ -42,7 +41,6 @@ function Annotation({
   isReply,
   onToggleReplies,
   replyCount,
-  showDocumentInfo,
   threadIsCollapsed,
   annotationsService,
 }) {
@@ -76,7 +74,6 @@ function Annotation({
             annotation={annotation}
             isEditing={isEditing}
             replyCount={replyCount}
-            showDocumentInfo={showDocumentInfo}
             threadIsCollapsed={threadIsCollapsed}
           />
 
@@ -130,7 +127,6 @@ Annotation.propTypes = {
   isReply: propTypes.bool,
   onToggleReplies: propTypes.func,
   replyCount: propTypes.number.isRequired,
-  showDocumentInfo: propTypes.bool.isRequired,
   threadIsCollapsed: propTypes.bool.isRequired,
   annotationsService: propTypes.object.isRequired,
 };

src/sidebar/components/Annotation/AnnotationDocumentInfo.js

 import propTypes from 'prop-types';
 
-import * as annotationMetadata from '../../helpers/annotation-metadata';
-
 /** @typedef {import("../../../types/api").Annotation} Annotation */
 
 /**
  * @typedef AnnotationDocumentInfoProps
- * @prop {Annotation} annotation - Annotation for which the document metadata will be rendered
+ * @prop {string} [domain] - The domain associated with the document
+ * @prop {string} [link] - A link to the document (directly)
+ * @prop {string} title - The document's title
  */
 
 /**
@@ -15,27 +15,17 @@ import * as annotationMetadata from '../../helpers/annotation-metadata';
  *
  * @param {AnnotationDocumentInfoProps} props
  */
-export default function AnnotationDocumentInfo({ annotation }) {
-  const documentInfo = annotationMetadata.domainAndTitle(annotation);
-  // If there's no document title, nothing to do here
-  if (!documentInfo.titleText) {
-    return null;
-  }
-
+export default function AnnotationDocumentInfo({ domain, link, title }) {
   return (
     <div className="AnnotationDocumentInfo u-layout-row u-horizontal-rhythm">
       <div className="AnnotationDocumentInfo__title u-color-text--muted">
         on &quot;
-        {documentInfo.titleLink ? (
-          <a href={documentInfo.titleLink}>{documentInfo.titleText}</a>
-        ) : (
-          <span>{documentInfo.titleText}</span>
-        )}
+        {link ? <a href={link}>{title}</a> : <span>{title}</span>}
         &quot;
       </div>
-      {documentInfo.domain && (
+      {domain && (
         <div className="AnnotationDocumentInfo__domain u-color-text--muted">
-          ({documentInfo.domain})
+          ({domain})
         </div>
       )}
     </div>
@@ -43,5 +33,7 @@ export default function AnnotationDocumentInfo({ annotation }) {
 }
 
 AnnotationDocumentInfo.propTypes = {
-  annotation: propTypes.object.isRequired,
+  domain: propTypes.string,
+  link: propTypes.string,
+  title: propTypes.string.isRequired,
 };

src/sidebar/components/Annotation/AnnotationHeader.js

@@ -4,6 +4,7 @@ import propTypes from 'prop-types';
 
 import { useStoreProxy } from '../../store/use-store';
 import {
+  domainAndTitle,
   isHighlight,
   isReply,
   hasBeenEdited,
@@ -26,9 +27,6 @@ import AnnotationUser from './AnnotationUser';
  * @prop {Annotation} annotation
  * @prop {boolean} [isEditing] - Whether the annotation is actively being edited
  * @prop {number} replyCount - How many replies this annotation currently has
- * @prop {boolean} [showDocumentInfo] -
- *   Should document metadata be rendered? Hint: this is enabled for single annotation
- *   and stream views.
  * @prop {boolean} threadIsCollapsed - Is this thread currently collapsed?
  */
 
@@ -43,7 +41,6 @@ export default function AnnotationHeader({
   annotation,
   isEditing,
   replyCount,
-  showDocumentInfo,
   threadIsCollapsed,
 }) {
   const store = useStoreProxy();
@@ -61,6 +58,13 @@ export default function AnnotationHeader({
   const showReplyButton = replyCount > 0 && isCollapsedReply;
   const showExtendedInfo = !isReply(annotation);
 
+  const annotationUrl = annotation.links?.html || '';
+  const documentInfo = domainAndTitle(annotation);
+  const documentLink =
+    annotationUrl && documentInfo.titleLink ? documentInfo.titleLink : '';
+  const showDocumentInfo =
+    store.route() !== 'sidebar' && documentInfo.titleText;
+
   const onReplyCountClick = () =>
     // If an annotation has replies it must have been saved and therefore have
     // an ID.
@@ -110,7 +114,11 @@ export default function AnnotationHeader({
             </div>
           )}
           {showDocumentInfo && (
-            <AnnotationDocumentInfo annotation={annotation} />
+            <AnnotationDocumentInfo
+              domain={documentInfo.domain}
+              link={documentLink}
+              title={documentInfo.titleText}
+            />
           )}
         </div>
       )}
@@ -122,6 +130,5 @@ AnnotationHeader.propTypes = {
   annotation: propTypes.object.isRequired,
   isEditing: propTypes.bool,
   replyCount: propTypes.number,
-  showDocumentInfo: propTypes.bool,
   threadIsCollapsed: propTypes.bool.isRequired,
 };

src/sidebar/components/Annotation/test/Annotation-test.js

@@ -35,7 +35,6 @@ describe('Annotation', () => {
         isReply={false}
         onToggleReplies={fakeOnToggleReplies}
         replyCount={0}
-        showDocumentInfo={false}
         threadIsCollapsed={true}
         {...props}
       />

src/sidebar/components/Annotation/test/AnnotationDocumentInfo-test.js

 import { mount } from 'enzyme';
 
-import * as fixtures from '../../../test/annotation-fixtures';
-
 import { checkAccessibility } from '../../../../test-util/accessibility';
-import mockImportedComponents from '../../../../test-util/mock-imported-components';
 
-import AnnotationDocumentInfo, { $imports } from '../AnnotationDocumentInfo';
+import AnnotationDocumentInfo from '../AnnotationDocumentInfo';
 
 describe('AnnotationDocumentInfo', () => {
-  let fakeDomainAndTitle;
-  let fakeMetadata;
-
   const createAnnotationDocumentInfo = props => {
     return mount(
       <AnnotationDocumentInfo
-        annotation={fixtures.defaultAnnotation()}
+        domain="www.foo.bar"
+        link="http://www.baz"
+        title="Turtles"
         {...props}
       />
     );
   };
 
-  beforeEach(() => {
-    fakeDomainAndTitle = sinon.stub();
-    fakeMetadata = { domainAndTitle: fakeDomainAndTitle };
-
-    $imports.$mock(mockImportedComponents());
-    $imports.$mock({
-      '../../helpers/annotation-metadata': fakeMetadata,
-    });
-  });
-
-  afterEach(() => {
-    $imports.$restore();
-  });
-
-  it('should not render if there is no document title', () => {
-    fakeDomainAndTitle.returns({});
-
-    const wrapper = createAnnotationDocumentInfo();
-    const info = wrapper.find('.AnnotationDocumentInfo');
-
-    assert.notOk(info.exists());
-  });
-
   it('should render the document title', () => {
-    fakeDomainAndTitle.returns({ titleText: 'I have a title' });
-
     const wrapper = createAnnotationDocumentInfo();
-    const info = wrapper.find('.AnnotationDocumentInfo');
+    const info = wrapper.find('.AnnotationDocumentInfo__title');
 
-    assert.isOk(info.exists());
+    assert.equal(info.text(), 'on "Turtles"');
   });
 
   it('should render a link if available', () => {
-    fakeDomainAndTitle.returns({
-      titleText: 'I have a title',
-      titleLink: 'https://www.example.com',
-    });
-
     const wrapper = createAnnotationDocumentInfo();
     const link = wrapper.find('.AnnotationDocumentInfo__title a');
 
-    assert.equal(link.prop('href'), 'https://www.example.com');
+    assert.equal(link.prop('href'), 'http://www.baz');
   });
 
   it('should render domain if available', () => {
-    fakeDomainAndTitle.returns({
-      titleText: 'I have a title',
-      domain: 'www.example.com',
-    });
-
     const wrapper = createAnnotationDocumentInfo();
     const domain = wrapper.find('.AnnotationDocumentInfo__domain');
 
-    assert.equal(domain.text(), '(www.example.com)');
+    assert.equal(domain.text(), '(www.foo.bar)');
   });
 
   it(
     'should pass a11y checks',
     checkAccessibility({
       content: () => {
-        fakeDomainAndTitle.returns({
-          titleText: 'I have a title',
-          domain: 'www.example.com',
-        });
         return createAnnotationDocumentInfo();
       },
     })

src/sidebar/components/Annotation/test/AnnotationHeader-test.js

@@ -8,6 +8,7 @@ import mockImportedComponents from '../../../../test-util/mock-imported-componen
 import AnnotationHeader, { $imports } from '../AnnotationHeader';
 
 describe('AnnotationHeader', () => {
+  let fakeDomainAndTitle;
   let fakeIsHighlight;
   let fakeIsReply;
   let fakeHasBeenEdited;
@@ -20,7 +21,6 @@ describe('AnnotationHeader', () => {
         annotation={fixtures.defaultAnnotation()}
         isEditing={false}
         replyCount={0}
-        showDocumentInfo={false}
         threadIsCollapsed={false}
         {...props}
       />
@@ -28,12 +28,14 @@ describe('AnnotationHeader', () => {
   };
 
   beforeEach(() => {
+    fakeDomainAndTitle = sinon.stub().returns({});
     fakeIsHighlight = sinon.stub().returns(false);
     fakeIsReply = sinon.stub().returns(false);
     fakeHasBeenEdited = sinon.stub().returns(false);
     fakeIsPrivate = sinon.stub();
 
     fakeStore = {
+      route: sinon.stub().returns('sidebar'),
       setExpanded: sinon.stub(),
     };
 
@@ -41,6 +43,7 @@ describe('AnnotationHeader', () => {
     $imports.$mock({
       '../../store/use-store': { useStoreProxy: () => fakeStore },
       '../../helpers/annotation-metadata': {
+        domainAndTitle: fakeDomainAndTitle,
         isHighlight: fakeIsHighlight,
         isReply: fakeIsReply,
         hasBeenEdited: fakeHasBeenEdited,
@@ -251,21 +254,84 @@ describe('AnnotationHeader', () => {
     });
 
     describe('annotation document info', () => {
-      it('should render document info if `showDocumentInfo` is enabled', () => {
-        const wrapper = createAnnotationHeader({ showDocumentInfo: true });
+      const fakeDocumentInfo = {
+        titleText: 'This document',
+        titleLink: 'http://www.example.com',
+        domain: 'www.foo.com',
+      };
+
+      beforeEach(() => {
+        fakeDomainAndTitle.returns(fakeDocumentInfo);
+      });
+
+      it('should not render document info if on sidebar route', () => {
+        fakeStore.route.returns('sidebar');
+        const wrapper = createAnnotationHeader();
 
         const documentInfo = wrapper.find('AnnotationDocumentInfo');
 
-        assert.isTrue(documentInfo.exists());
+        assert.isFalse(documentInfo.exists());
       });
 
-      it('should not render document info if `showDocumentInfo` is not enabled', () => {
-        const wrapper = createAnnotationHeader({ showDocumentInfo: false });
+      it('should not render document info if document does not have a title', () => {
+        fakeStore.route.returns('notebook');
+        fakeDomainAndTitle.returns({});
+
+        const wrapper = createAnnotationHeader();
 
         const documentInfo = wrapper.find('AnnotationDocumentInfo');
 
         assert.isFalse(documentInfo.exists());
       });
+
+      [
+        {
+          route: 'notebook',
+          documentInfo: fakeDocumentInfo,
+          expectedPresence: true,
+        },
+        { route: 'notebook', documentInfo: {}, expectedPresence: false },
+        {
+          route: 'sidebar',
+          documentInfo: fakeDocumentInfo,
+          expectedPresence: false,
+        },
+      ].forEach(testCase => {
+        it('should render document info if document info available and not on sidebar route', () => {
+          fakeStore.route.returns(testCase.route);
+          fakeDomainAndTitle.returns(testCase.documentInfo);
+
+          const wrapper = createAnnotationHeader();
+          const documentInfo = wrapper.find('AnnotationDocumentInfo');
+
+          assert.equal(documentInfo.exists(), testCase.expectedPresence);
+        });
+      });
+
+      it('should set document properties as props to `AnnotationDocumentInfo`', () => {
+        fakeStore.route.returns('notebook');
+        const wrapper = createAnnotationHeader();
+
+        const documentInfo = wrapper.find('AnnotationDocumentInfo');
+
+        assert.isTrue(documentInfo.exists());
+        assert.equal(documentInfo.props().title, 'This document');
+        // Link is not set because Annotation prop (default fixture) doesn't
+        // have a URL (html link)
+        assert.equal(documentInfo.props().link, '');
+        assert.equal(documentInfo.props().domain, 'www.foo.com');
+      });
+
+      it('should provide document link for document info if annotation has an HTML link/URL', () => {
+        const annotation = fixtures.defaultAnnotation();
+        annotation.links = { html: 'http://www.whatever' };
+        fakeStore.route.returns('notebook');
+        const wrapper = createAnnotationHeader({ annotation });
+
+        const documentInfo = wrapper.find('AnnotationDocumentInfo');
+
+        assert.equal(documentInfo.props().link, 'http://www.example.com');
+      });
     });
   });
 

src/sidebar/components/Thread.js

@@ -14,7 +14,6 @@ import ModerationBanner from './ModerationBanner';
 
 /**
  * @typedef ThreadProps
- * @prop {boolean} [showDocumentInfo]
  * @prop {Thread} thread
  * @prop {Object} threadsService - Injected service
  */
@@ -26,7 +25,7 @@ import ModerationBanner from './ModerationBanner';
  *
  * @param {ThreadProps} props
  */
-function Thread({ showDocumentInfo = false, thread, threadsService }) {
+function Thread({ thread, threadsService }) {
   // Render this thread's replies only if the thread is expanded
   const showChildren = !thread.collapsed;
 
@@ -68,7 +67,6 @@ function Thread({ showDocumentInfo = false, thread, threadsService }) {
             isReply={!!thread.parent}
             onToggleReplies={onToggleReplies}
             replyCount={thread.replyCount}
-            showDocumentInfo={showDocumentInfo}
             threadIsCollapsed={thread.collapsed}
           />
         </>
@@ -76,7 +74,6 @@ function Thread({ showDocumentInfo = false, thread, threadsService }) {
     [
       hasAppliedFilter,
       onToggleReplies,
-      showDocumentInfo,
       thread.annotation,
       thread.parent,
       thread.replyCount,
@@ -129,7 +126,6 @@ function Thread({ showDocumentInfo = false, thread, threadsService }) {
 }
 
 Thread.propTypes = {
-  showDocumentInfo: propTypes.bool,
   thread: propTypes.object.isRequired,
 
   // Injected

src/sidebar/components/ThreadCard.js

@@ -28,7 +28,6 @@ function ThreadCard({ frameSync, thread }) {
   const store = useStoreProxy();
   const threadTag = thread.annotation && thread.annotation.$tag;
   const isFocused = store.isAnnotationFocused(threadTag);
-  const showDocumentInfo = store.route() !== 'sidebar';
   const focusThreadAnnotation = useMemo(
     () =>
       debounce(tag => {
@@ -57,10 +56,7 @@ function ThreadCard({ frameSync, thread }) {
 
   // Memoize threads to reduce avoid re-rendering when something changes in a
   // parent component but the `Thread` itself has not changed.
-  const threadContent = useMemo(
-    () => <Thread thread={thread} showDocumentInfo={showDocumentInfo} />,
-    [thread, showDocumentInfo]
-  );
+  const threadContent = useMemo(() => <Thread thread={thread} />, [thread]);
 
   return (
     /* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */

src/sidebar/components/test/Thread-test.js

@@ -70,7 +70,6 @@ describe('Thread', () => {
   const createComponent = props => {
     return mount(
       <Thread
-        showDocumentInfo={false}
         thread={createThread()}
         threadsService={fakeThreadsService}
         {...props}

src/sidebar/components/test/ThreadCard-test.js

@@ -58,22 +58,6 @@ describe('ThreadCard', () => {
     assert(wrapper.find('.ThreadCard').hasClass('is-focused'));
   });
 
-  it('shows document info if current route is not sidebar', () => {
-    fakeStore.route.returns('whatever');
-
-    const wrapper = createComponent();
-
-    assert.isTrue(wrapper.find('Thread').props().showDocumentInfo);
-  });
-
-  it('does not show document info if current route is sidebar', () => {
-    fakeStore.route.returns('sidebar');
-
-    const wrapper = createComponent();
-
-    assert.isFalse(wrapper.find('Thread').props().showDocumentInfo);
-  });
-
   describe('mouse and click events', () => {
     it('scrolls to the annotation when the `ThreadCard` is clicked', () => {
       const wrapper = createComponent();