Add section for deactivated API release notes #413

maddy-jo · 2020-08-28T20:13:00Z

Description

completes API-1790. design ticket is API-1495.

adds a new section showing release notes for deactivated APIs. tests have been added using React Testing Library in support of API-1317.

Process

Tests added or updated for change
- added tests with React Testing Library
Docs updated
Accessibility concerns have been considered and addressed
- no structural changes, tightened up an ARIA label or so

Requested feedback

…mponent

maddy-jo · 2020-09-02T18:11:42Z

package.json

@@ -132,6 +134,7 @@
    "postcss-preset-env": "6.0.6",
    "postcss-safe-parser": "4.0.1",
    "prettier": "^1.16.4",
+    "pretty-format": "^26.4.2",


issue with requiring pretty-format from @testing-library/react: testing-library/react-testing-library#694

src/App.tsx

maddy-jo · 2020-09-02T18:14:06Z

src/apiDefs/data/categories.ts

    content: appealsContent,
    name: 'Appeals API',
    properName: 'Appeals API',
  },
  benefits: {
    apis: benefitsApis,
-    buttonText: 'Get Your Key',


been meaning to take this thingy out for a while. i think it used to be on the home page but hasn't been for like a year

maddy-jo · 2020-09-02T18:15:52Z

src/apiDefs/query.ts

@@ -20,7 +20,7 @@ const getApiDefinitions = () => apiDefs;
 const getApiCategoryOrder = () => apiCategoryOrder;

 const getAllApis = (): IApiDescription[] => {
-  return Object.values(apiDefs).flatMap((category: IApiCategory) => category.apis);
+  return Object.values(getApiDefinitions()).flatMap((category: IApiCategory) => category.apis);


wanted not to reference apiDefs directly in multiple places. need to prepare to use a real database and corresponding data repository layer for these definitions in the future.

va-bot · 2020-09-02T18:18:23Z

These changes have been deployed to an S3 bucket. A build for each environment is available:

https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/8b4f787/dev/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/8b4f787/staging/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/8b4f787/production/index.html

Due to S3 website hosting limitations in govcloud you need to first navigate to index.html explicitly.

src/components/PageHeader.tsx

maddy-jo · 2020-09-02T19:48:25Z

src/containers/releaseNotes/CategoryReleaseNotes.tsx

@@ -0,0 +1,136 @@
+import AlertBox from '@department-of-veterans-affairs/formation-react/AlertBox';


renamed this file to get away from vague use of the language of "page"

… rule is disabled entirely

maddy-jo · 2020-09-02T19:55:29Z

src/containers/releaseNotes/CategoryReleaseNotes.tsx

+const ReleaseNotesCardLinks = (props: ReleaseNotesCardLinksProps) => {
+  const { apiCategory, categoryKey, flagName } = props;
+  const flags: { [apiId: string]: boolean } = getFlags()[flagName];
+  const apis: IApiDescription[] = apiCategory.apis.filter(api => flags[api.urlFragment]);


we can't use Flag for this because we only want to show these card links if there is more than one API that will actually be shown on the page, not more than one API in the category, period. if the category has 4 APIs but 3 of them are disabled or deactivated, we don't want this component to return anything. getFlags() is a good substitute for <Flag />, now that it's public.

va-bot · 2020-09-02T20:02:25Z

These changes have been deployed to an S3 bucket. A build for each environment is available:

https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/7cdf216/dev/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/7cdf216/staging/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/7cdf216/production/index.html

Due to S3 website hosting limitations in govcloud you need to first navigate to index.html explicitly.

Eallan919 · 2020-09-02T21:18:18Z

Looking good! Can we get a space after the "," and contact us? under the relase note itself, please?

maddy-jo · 2020-09-02T21:31:44Z

@Eallan919 thanks for the catch! i think that whole release note should actually be update so it's one line in the markdown file and wraps properly... hmm

…deactivation

va-bot · 2020-09-02T21:47:20Z

These changes have been deployed to an S3 bucket. A build for each environment is available:

https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/b1c5152/dev/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/b1c5152/staging/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/b1c5152/production/index.html

Due to S3 website hosting limitations in govcloud you need to first navigate to index.html explicitly.

charleystran · 2020-09-10T18:37:00Z

.env.production

@@ -8,3 +8,4 @@ REACT_APP_TRY_IT_OUT_ENABLED=false
 REACT_APP_SENTRY_DSN=https://4409b200977e4429bb210b601439ef0a@developer.va.gov/sentry/17
 REACT_APP_SENTRY_RELEASE=$SENTRY_RELEASE
 REACT_APP_SHOW_TESTING_NOTICE=false
+REACT_APP_ARGONAUT_API_ENABLED=false


Wondering what this change means as it related to this PR, or is this cleaning up a missing setting?

for other reviewers: the Argonaut API's release notes aren't very meaningful because they're from an earlier iteration of the release notes where we were less consistent about updating them. as a result, when we decided to show release notes for deactivated APIs, UX asked us to hide those, for now at least. you can see the release notes in the source file or in the dev and staging review instances. (i'm choosing to leave it on by default for those other environments - i don't want to set a precedent for getting rid of inconvenient things from deactivated APIs wholesale by marking them disabled in addition to deactivated.)

va-bot · 2020-09-10T18:40:59Z

These changes have been deployed to an S3 bucket. A build for each environment is available:

https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/af6df93/dev/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/af6df93/staging/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/af6df93/production/index.html

Due to S3 website hosting limitations in govcloud you need to first navigate to index.html explicitly.

anthony-kopczyk

This is good. I have a couple questions, probably mostly because I'm new. There is language on two tests that I think could be reworded more explicitly. I'm also unsure about the ReleaseNotes Route, render/component attributes. Someone that knows the app more would be better to look at that specific piece.

I don't see anything that needs changed right away but don't want to approve until I know about that React Route component render/component stuff. - granted, I could probably approve. Even if it should be the other way I don't see it breaking anything.

src/App.tsx

anthony-kopczyk · 2020-09-11T13:50:53Z

src/components/PageHeader.tsx

  className?: string;
  description?: string;
  halo?: string;
  header: string;
  id?: string;
+  containerId?: string;


I didn't know optional properties were a thing until now. That's neat.

src/components/PageHeader.tsx

anthony-kopczyk · 2020-09-11T13:54:49Z

src/components/PageHeader.tsx

@@ -3,32 +3,27 @@ import * as React from 'react';

 import './PageHeader.scss';

-interface IPageHeaderProps {
+interface PageHeaderProps {


Is moving away from the 'I' prefix something we're explicitly doing as a whole project?

I agree with the change but curious if this was a high level shared decision. - so when I make my first code changes I can follow the right convention.

yeah, definitely - should've left a comment on this, because this question has come up before recently and there's a big difference between a yes and no answer. yes, we decided to go with no prefix around the time the backend got stood up as its own Node app, and we decided to be consistent across the front and back end. apologies for making an assumption here!

anthony-kopczyk · 2020-09-11T13:57:51Z

src/containers/releaseNotes/ReleaseNotesOverview.tsx

@@ -33,6 +45,11 @@ export default () => {
            </Flag>
          );
        })}
+        {deactivatedCategory.apis.length > 0 && (
+          <CardLink name={deactivatedCategory.name} url="/release-notes/deactivated">
+            This is a repository for deactivated APIs and related documentation and release notes.


When this says 'this is a repository', is that like a git repository that should be linked, or is it meaning the card link is a place where they can find all deactivated apis in the developer-portal webapp?

good question - it's the latter. sometimes the release notes link out to PRs and whatnot, but this isn't intended to be in the technical version control sense.

anthony-kopczyk · 2020-09-11T14:04:46Z

src/containers/releaseNotes/ReleaseNotes.tsx

-      }
-    });
-  };
+interface SideNavCategoryEntryProps {


I like the code clean up with this.

anthony-kopczyk · 2020-09-11T14:19:41Z

src/containers/releaseNotes/ReleaseNotesOverview.test.tsx

+      expect(cardLink.getAttribute('href')).toBe('/release-notes/sports');
+    });
+
+    it('does not render a card for a disabled category', () => {


I like this test. I feel like this kind of check is the kind that would usually get left out. Thanks for adding it!

anthony-kopczyk · 2020-09-11T14:20:38Z

src/containers/releaseNotes/ReleaseNotesOverview.test.tsx

+      ).toBeNull();
+    });
+
+    it('has a card link for deactivated APIs if there are any', () => {


Maybe rephrase this to 'has a card link for deactivated APIs when one exists'? I was trying to figure out if this test was somehow covering both conditions.

anthony-kopczyk · 2020-09-11T14:23:26Z

src/containers/releaseNotes/ReleaseNotes.tsx

-                  path="/release-notes/:apiCategoryKey"
-                  component={CategoryReleaseNotesPage}
+                  path="/release-notes/deactivated"
+                  render={DeactivatedReleaseNotes}


I am just now looking up the difference between using render and component in a Route path. I'd like a second set of eyes on this to say if it's good. - or if you could explain it for me why we are good to use render on DeactivedReleaseNotes here but use component for CategoryReleaseNotes below.

wow, great catch. i'm not really sure when/why i made this render, it should probably be component. i believe they're functionally equivalent in this case, but render is more frequently used for inline functions.

When I was looking it up I saw something about one of them making new instances or something, but I'm not sure specifics or how these components are used so not sure at all. haha.

yeahhh fair, i think the differences are pretty hard to parse out. i think the idea is that <Route component={() => <div>hello!</div>} /> will mount a new component each time it gets rendered because of how it treats that function internally (maybe because it's anonymous? i don't know either, would have to research it more to get what's happening there). but if you do <Route render={() => <div>hello!</div>} />, React Router treats it differently and therefore handles that case.

anthony-kopczyk · 2020-09-11T14:25:10Z

src/containers/releaseNotes/ReleaseNotes.test.tsx

+        expect(queryByRole(sideNav, 'link', { name: 'Sports API' })).toBeNull();
+      });
+
+      it('has an entry for deactivated APIs', () => {


Similar to ReleaseNotesOverviewTest this could be reworded to be something like 'has an entry for deactived APIs when there are deactived APIs'

yup, that's a good point, thanks! will clarify that language

va-bot · 2020-09-11T18:08:39Z

These changes have been deployed to an S3 bucket. A build for each environment is available:

https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/e6ca0d6/dev/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/e6ca0d6/staging/index.html
https://s3-us-gov-west-1.amazonaws.com/review-developer-va-gov/e6ca0d6/production/index.html

Due to S3 website hosting limitations in govcloud you need to first navigate to index.html explicitly.

anthony-kopczyk

Looks good. The previous reviews were addressed.

charleystran

LGTM - Good Job on this!

stephennovosel added 30 commits August 3, 2020 15:19

deactivated apis page base created

6f57410

banner created for content

879daf5

deactivated apis moved from healh apiDefs to deactivated

aff71f7

refactor to use existing object structure and components

5adc851

add test for deactivation info

7363914

updated snapshots

15d4aa2

change classnames to single quote and deactivatedApis to camelcase

6c194fb

revert health api definition to original array

b8f5d24

remove deactivated apis from categories

de0694e

remove deactivated from categories

f5aaa87

revert category page to original

7fce1c8

use synthetic deactivated category

30fdc0e

updated snapshots'

30dc059

revert urgent care notice markdown

d2c6815

revert categories page

6a967f7

revert health.ts

98c932e

reverting test

02c8a0a

Merge branch 'master' into API-1790/add-deactivation-apis

6b6477d

passing flagName via props rather than determining from within the co…

6cff002

…mponent

remove deactivated category from apiDefs

8b06412

refactoring for readability

feedfe7

Merge branch 'master' into API-1790/add-deactivation-apis

bb08e06

refactor to use custom hook and tests

da5b476

handle merge conflictst

2ef078f

reverting snapshots to original

db0bf5f

revert category file

748e83b

revert health apiDefs file

16bfe15

revert query.test.ts

f2333d9

revert query test file

0093897

attempt to revert query test file

b65d107

Merge branch 'master' into API-1790

8b4f787

maddy-jo commented Sep 2, 2020

View reviewed changes

src/App.tsx Show resolved Hide resolved

maddy-jo commented Sep 2, 2020

View reviewed changes

src/components/PageHeader.tsx Show resolved Hide resolved

maddy-jo commented Sep 2, 2020

View reviewed changes

Remove TSLint disable statements for interface-name rule now that the…

7cdf216

… rule is disabled entirely

maddy-jo commented Sep 2, 2020

View reviewed changes

Fix formatting of Urgent Care release note from 7/21/20 about the UC …

b1c5152

…deactivation

maddy-jo mentioned this pull request Sep 2, 2020

Add unit tests for News component #419

Merged

3 tasks

Merge branch 'master' into API-1790

af6df93

charleystran reviewed Sep 10, 2020

View reviewed changes

anthony-kopczyk self-assigned this Sep 11, 2020

anthony-kopczyk reviewed Sep 11, 2020

View reviewed changes

Mike Lumetta added 2 commits September 11, 2020 13:56

Merge remote-tracking branch 'origin/master' into API-1790

03580b0

Respond to feedback

e6ca0d6

maddy-jo requested review from charleystran and anthony-kopczyk September 11, 2020 17:59

anthony-kopczyk approved these changes Sep 11, 2020

View reviewed changes

charleystran approved these changes Sep 11, 2020

View reviewed changes

maddy-jo merged commit 8a757e0 into master Sep 11, 2020

maddy-jo deleted the API-1790 branch September 11, 2020 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add section for deactivated API release notes #413

Add section for deactivated API release notes #413

maddy-jo commented Aug 28, 2020

maddy-jo Sep 2, 2020 •

edited

maddy-jo Sep 2, 2020

maddy-jo Sep 2, 2020

va-bot commented Sep 2, 2020

maddy-jo Sep 2, 2020

maddy-jo Sep 2, 2020

va-bot commented Sep 2, 2020

Eallan919 commented Sep 2, 2020

maddy-jo commented Sep 2, 2020 •

edited

va-bot commented Sep 2, 2020

charleystran Sep 10, 2020

maddy-jo Sep 11, 2020

va-bot commented Sep 10, 2020

anthony-kopczyk left a comment

anthony-kopczyk Sep 11, 2020

anthony-kopczyk Sep 11, 2020

maddy-jo Sep 11, 2020

anthony-kopczyk Sep 11, 2020

maddy-jo Sep 11, 2020

anthony-kopczyk Sep 11, 2020

anthony-kopczyk Sep 11, 2020

anthony-kopczyk Sep 11, 2020

anthony-kopczyk Sep 11, 2020

maddy-jo Sep 11, 2020

anthony-kopczyk Sep 11, 2020

maddy-jo Sep 11, 2020

anthony-kopczyk Sep 11, 2020

maddy-jo Sep 11, 2020

va-bot commented Sep 11, 2020

anthony-kopczyk left a comment

charleystran left a comment

		@@ -0,0 +1,136 @@
		import AlertBox from '@department-of-veterans-affairs/formation-react/AlertBox';

Add section for deactivated API release notes #413

Add section for deactivated API release notes #413

Conversation

maddy-jo commented Aug 28, 2020

Description

Process

Requested feedback

maddy-jo Sep 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

va-bot commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

va-bot commented Sep 2, 2020

Eallan919 commented Sep 2, 2020

maddy-jo commented Sep 2, 2020 • edited

va-bot commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

va-bot commented Sep 10, 2020

anthony-kopczyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

va-bot commented Sep 11, 2020

anthony-kopczyk left a comment

Choose a reason for hiding this comment

charleystran left a comment

Choose a reason for hiding this comment

maddy-jo Sep 2, 2020 •

edited

maddy-jo commented Sep 2, 2020 •

edited