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

Adds per library book un/suppression. (PP-985) #120

Merged
merged 5 commits into from
Jul 11, 2024

Conversation

tdilauro
Copy link
Contributor

@tdilauro tdilauro commented Jul 2, 2024

Description

Adds the ability to suppress/unsuppress book visibility at the library level in the book detail editor.

Motivation and Context

[Jira PP-985]

How Has This Been Tested?

Checklist:

  • N/A - I have updated the documentation accordingly.
  • All new and existing tests passed.

@tdilauro tdilauro requested a review from ray-lee July 2, 2024 13:22
Copy link
Contributor

@ray-lee ray-lee left a comment

Choose a reason for hiding this comment

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

Just a few comments/questions below.

src/components/BookDetailsEditorSuppression.tsx Outdated Show resolved Hide resolved
src/components/ConfirmationModalWithOutcome.tsx Outdated Show resolved Hide resolved
src/editorAdapter.ts Outdated Show resolved Hide resolved
title = undefined,
type = undefined,
} = link;
return { ...link, href, rel, title, type, role };
Copy link
Contributor

Choose a reason for hiding this comment

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

LinkData is defined as:

export interface LinkData {
href: string;
rel: string;
}

Why do we need anything besides href and rel in the returned object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might not. However, I included them because the objects that were returned previously were OPDSLinks, which do have those properties. And, if I'm remembering correctly, some of the test set/use those other properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ray-lee I went back and tested the following:

const opdsLinkToLinkData = (link: OPDSLink | undefined): LinkData => {
  if (!link) {
    return link;
  }
  const { href, rel } = link;
  return { href, rel };
};

and got a failing test here:

  1 failing

  1) editorAdapter
       adapts valid OPDS entry:

      AssertionError: expected { Object (href, rel) } to deeply equal { Object (href, rel, ...) }
      + expected - actual

       {
         "href": "hide"
         "rel": "http://librarysimplified.org/terms/rel/hide"
      +  "role": "role"
      +  "title": "title"
      +  "type": "type"
       }
      
      at Context.<anonymous> (src/__tests__/editorAdapter-test.ts:140:38)
      at processImmediate (node:internal/timers:476:21)

But I could at least avoid explicitly mentioning role, title, and type here by changing the last two lines to

  const { href, rel } = link;
  return { ...link, href, rel };

Do you have a preference among these options?

  1. Leave the code as it is;
  2. Use the last couple of lines, as above (return OPDSLink props besides rel and href); or
  3. Drop the failing test, assuming that there is no functionality that depends on those other properties being present on the object, and return on rel and href.

If I leave it as is, I can update the LinkData interface to acknowledge the presence of these other properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably just add the properties to the LinkData interface, so that the expectation that they are present is explicit. It sounds like we're not sure if that expectation is only in a test. If we were more sure, I'd go with option 3.

@tdilauro
Copy link
Contributor Author

@ray-lee This one's ready for another look when you have time.

Copy link
Contributor

@ray-lee ray-lee left a comment

Choose a reason for hiding this comment

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

Looks good!

@tdilauro tdilauro merged commit ee38ecf into main Jul 11, 2024
1 check passed
@tdilauro tdilauro deleted the feature/per-library-hide-unhide branch July 11, 2024 19:07
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.

2 participants