DOP-3903: API Changelog Support to omit entries #919

anabellabuckvar · 2023-09-26T15:08:10Z

Stories/Links:

Current Behavior:

Staging Links:

Notes:

Atlas App Services Function fetchOADiff was updated in the process of completing this ticket, please review it as well before giving a LGTM!

caesarbell

Is there a way you can provide a staging link to verify the success of this PR?

caesarbell · 2023-09-28T19:27:12Z

I am sorry, I just noticed this PR is a Draff.

branberry

Overall, this is really good! I left some comments about some of the checks, and a small concern I had about the updating of values in the map calls.

Also, I would recommend adding unit tests to ensure that it's working as expected.

branberry · 2023-10-03T12:54:27Z

gatsby-node.js

+
+    const hideChanges = (changelog) => {
+      const versionUpdate = (version) => {
+        if (version !== null && version.changes !== null) {


Updated if statement

I don't believe version will be null, so we can omit the check here. Also, for version.changes, you'll want to check if it's undefined. The simplest way to do this is to check if it is falsy (if (!version.changes))

JavaScript is weird in that there is both null and undefined which aren't exactly the same thing, even though the behavior is similar. More often than not, if we want to check if a property exists, we will want to check if it is undefined as opposed to null. Another option is to simply check if the variable or property is falsy. This means in the if statement you can do something like this:

if (version && version.changes) { .... }

JavaScript will convert !version and !version.changes to a boolean value. If the value (for example, version), is null, undefined, 0, NaN, or ''(an empty string), the if statement will coerce these values tofalse`.

Here's a helpful link to describe what I am referring to in more detail

branberry · 2023-10-03T13:47:36Z

gatsby-node.js

+
+      //pathUpdate takes the array of versions from the specific path passed in and takes each version and runs versionUpdate on it
+      const pathUpdate = (path) => {
+        path.versions = path.versions.map(versionUpdate);


Mutating the current object that is provided in the map could lead to unintended side effects. Just to err on the side of caution, you can make a copy of path and update that. You can make a copy like so:

const updatedPath = {...path}

branberry · 2023-10-03T13:49:28Z

gatsby-node.js

+      const dateUpdate = (dateSection) => {
+        dateSection.paths = dateSection.paths.map(pathUpdate);
+        dateSection.paths = dateSection.paths.filter(
+          (path) => path !== null && path.versions !== null && path.versions.length !== 0


We can omit path !== null, we'll want to also check if path.versions is also undefined

branberry · 2023-10-03T13:49:52Z

gatsby-node.js

+      //changelog is the json file with everything in it. Map at this level takes each date section and runs dateUpdate on it
+      changelog = changelog.map(dateUpdate);
+      return changelog.filter(
+        (dateSection) => dateSection !== null && dateSection.paths !== null && dateSection.paths.length !== 0


We can omit dateSection !== null, we'll want to also check if dateSection.paths is also undefined

branberry · 2023-10-03T13:50:06Z

src/components/OpenAPIChangelog/OpenAPIChangelog.js

@@ -72,6 +72,15 @@ const StyledLoadingSkeleton = styled.div`
 const OpenAPIChangelog = () => {
  const { snootyEnv } = useSiteMetadata();
  const { index = {}, changelog = [], changelogResourcesList = [] } = useChangelogData();
+  //updatedChangeLog = changelog.filter()
+  //console.log(changelog);


This can be removed

branberry · 2023-10-03T13:52:01Z

src/components/OpenAPIChangelog/utils/useFetchDiff.js

@@ -3,10 +3,22 @@ import { fetchOADiff } from '../../../utils/realm';
 import useChangelogData from '../../../utils/use-changelog-data';
 import { getDiffRequestFormat } from './getDiffRequestFormat';

+//filter hideFromChangelog in diff
+const hideDiffChanges = (diffData) => {
+  const pathUpdate = (path) => {


Logic looks like it's the same from the filtering of the changelog in gatsby-node.js. Maybe this function could be put into another file and then imported where needed.

mmeigs

Mainly a comment below agreeing and expanding on what Brandon mentioned!
Also, just reminding here that this merge with the latest master will be an odd one. This will mostly all go in plugins/utils/openapi I would guess.

mmeigs · 2023-10-03T14:40:11Z

gatsby-node.js

+
+    const changelog = hideChanges(unfilteredChangelog);


I think everything @branberry said is true!! Here's one way to go about it easily and combine all he said.

For each spot where you're mapping (and changing by reference an entire object) you could change this each time to a safe action such as Brandon said for all of them, ie:

path.versions = path.versions.map(versionUpdate);

would become

const updatedPath = { ...path, versions: path.versions.map(versionUpdate) }

and then return the updatedPath rather than the path

Then, for each of these (four?) spots where you're filtering, you can condense your three statements (truthy property, truthy nested property, and not an empty array) to something like

path.versions.filter((version) => version.changes?.length !== 0);

For absolute extra security, you could put another optional chain behind version, ie version?.changes?.length but that would be so odd and a very bad error on APIx's part if they had an array that included nulls or empties.

mmeigs · 2023-10-03T14:42:48Z

src/components/OpenAPIChangelog/utils/useFetchDiff.js

 export const useFetchDiff = (resourceVersionOne, resourceVersionTwo, setIsLoading, setToastOpen, snootyEnv) => {
  const { index = {}, mostRecentDiff = {} } = useChangelogData();
  const [diff, setDiff] = useState([]);
-


nit: I'm ridiculous but I like this empty line for my brain to separate :)

mmeigs · 2023-10-03T14:44:20Z

src/components/OpenAPIChangelog/utils/useFetchDiff.js

-          setDiff(response);
+          setDiff(hideDiffChanges(response));


Total nit and stylistic choice, but for readability, I prefer to separate two invocations of functions. Maybe:

const filteredDiff = hideDiffChanges(response); setDiff(filteredDiff);

branberry

LGTM!

mmeigs · 2023-10-03T17:30:18Z

gatsby-node.js

+    const hideDiffChanges = (diffData) => {
+      const pathUpdate = (path) => {
+        const updatedPath = { ...path };
+        if (path?.changes) {


We shouldn't need an optional chain here!

mmeigs · 2023-10-03T17:30:35Z

gatsby-node.js

+      };
+
+      const updatedDiffData = diffData.map(pathUpdate);
+      return updatedDiffData.filter((path) => path?.changes?.length);


Same here, no need for the optional chain on path, but yes, keep the one on changes!

mmeigs · 2023-10-05T13:45:49Z

tests/unit/__snapshots__/OpenAPIChangelog.test.js.snap

+            <div
+              class="emotion-57 emotion-58 emotion-59"
+            >
+              Removed
+            </div>
+          </div>
+          <ul
+            class="emotion-60 emotion-61"
+          >
+            <li
+              class="emotion-62 emotion-63"
+            >
+              Resource version 2023-01-01 was removed, latest available resource version is 2023-02-01
+            </li>
+          </ul>
+        </div>
+      </section>


Hmm, this makes me slightly worried? I believe this is the change you added that has the hideFromChangelog flag as true? So, it shouldn't be in the snapshot

mmeigs · 2023-10-05T13:46:04Z

tests/unit/__snapshots__/ChangeList.test.js.snap

+            Removed
+          </div>
+        </div>
+        <ul
+          class="emotion-17 emotion-18"
+        >
+          <li
+            class="emotion-19 emotion-20"
+          >
+            Resource version 2023-01-01 was removed, latest available resource version is 2023-02-01
+          </li>
+        </ul>
+      </div>
+    </section>


I redid the tests, the snapshots should be clearer now.

mmeigs · 2023-10-05T13:48:06Z

plugins/utils/openapi.js

+    const hideDiffChanges = (diffData) => {
+      const pathUpdate = (path) => {
+        const updatedPath = { ...path };
+        if (path?.changes) {
+          updatedPath.changes = path.changes.filter((change) => !change.hideFromChangelog);
+        }
+        return updatedPath;
+      };
+
+      const updatedDiffData = diffData.map(pathUpdate);
+      return updatedDiffData.filter((path) => path.changes?.length);
+    };


Oh, and reminder to have this function somewhere where it can be exported so we can reuse it rather than declaring it twice!

Refactored the functions, let me know what you think!

mmeigs

Two main things about placement of hiding the changes!

You can/should use the hideChanges util function in gatsby-node! That way it only occurs once, server-side.
We still need to use hideDiffChanges in gatsby-node as well. Otherwise, the most recent diff will not have the changes hidden.

mmeigs · 2023-10-06T18:40:52Z

tests/unit/ChangeList.test.js

+  // it('does not render all version changelog changes with "hideFromChangelog=true"', () => {
+  //   const hiddenChanges = mockChangelog.reduce((acc, date) => {
+  //     date.paths.forEach((path) =>
+  //       path.versions.forEach((version) =>
+  //         version.changes.forEach((change) => change.hideFromChangelog && acc.push(change))
+  //       )
+  //     );
+  //     return acc;
+  //   }, []);
+
+  //   const { queryByText } = render(<ChangeList changes={mockChangelog} versionMode={ALL_VERSIONS} />);
+
+  //   expect(hiddenChanges).toHaveLength(2);
+
+  //   hiddenChanges.forEach((change) => {
+  //     expect(queryByText(change.change)).toBeNull();
+  //   });
+  // });
+
+  // it('does not render Diff changelog changes with "hideFromChangelog=true"', () => {
+  //   const hiddenChanges = mockDiff.reduce((acc, resource) => {
+  //     resource.changes.forEach((change) => change.hideFromChangelog && acc.push(change));
+  //     return acc;
+  //   }, []);
+
+  //   const { queryByText } = render(<ChangeList changes={mockDiff} versionMode={COMPARE_VERSIONS} />);
+
+  //   expect(hiddenChanges).toHaveLength(2);
+
+  //   hiddenChanges.forEach((change) => {
+  //     expect(queryByText(change.change)).toBeNull();
+  //   });
+  // });


These can be removed, I believe

mmeigs · 2023-10-06T18:45:54Z

src/components/OpenAPIChangelog/utils/filterHiddenChanges.js

@@ -0,0 +1,41 @@
+export const hideChanges = (changelog) => {


Would also love a comment above this explaining what this first util function is doing (ie what field it's hiding and that it is meant to be used on the changelog rather than the diff

mmeigs · 2023-10-06T18:47:56Z

tests/unit/filterHiddenChanges.test.js

+  },
+}));
+
+describe(' ChangeList data', () => {


I think all of this is fine! I also think if you wanted, this could just be a utility function test, so no need to render any components. Just pass a changelog and a diff through the functions and make sure the output is correct. But I'm totally fine with leaving all this. It accomplishes the same goal!

mmeigs

It looks good overall. But I am worried about using CommonJS exports in the frontend/react. Does this actually work? Especially if you comment out lines 17-20 in useFetchDiff?

anabellabuckvar · 2023-10-09T19:56:15Z

It looks good overall. But I am worried about using CommonJS exports in the frontend/react. Does this actually work? Especially if you comment out lines 17-20 in useFetchDiff?

Yes, it worked when I commented them out, and I used CommonJs exports only because I was following the format I saw in some of the other files. Would you like me to change it @mmeigs

mmeigs

Ok, LGTM!

anabellabuckvar and others added 2 commits September 26, 2023 11:07

DOP-3938 functional

cd1b530

Merge branch 'master' into DOP-3903-b

73184b4

anabellabuckvar changed the title ~~DOP-3938: API Changelog Support to omit entries~~ DOP-3903: API Changelog Support to omit entries Sep 26, 2023

anabellabuckvar added 2 commits September 26, 2023 11:36

DOP-3903-b cleaning up

00dedcb

DOP-3903-b hidefromchangelog frontend diff

6f8af87

anabellabuckvar requested a review from mmeigs September 26, 2023 19:49

caesarbell reviewed Sep 28, 2023

View reviewed changes

anabellabuckvar added 2 commits October 2, 2023 15:19

DOP-3903-b tested changes

6f341b2

DOP-3903-b deleted comments

3d90d72

anabellabuckvar requested review from caesarbell, branberry and schmalliso October 2, 2023 19:28

anabellabuckvar marked this pull request as ready for review October 2, 2023 21:02

branberry requested changes Oct 3, 2023

View reviewed changes

branberry self-requested a review October 3, 2023 14:59

mmeigs reviewed Oct 3, 2023

View reviewed changes

anabellabuckvar added 2 commits October 3, 2023 11:14

DOP-3903-b boolean and object copies refactoring

7ef99b6

DOP-3903-b stylistic changes

7bac35e

anabellabuckvar removed the request for review from caesarbell October 3, 2023 17:31

anabellabuckvar added 5 commits October 3, 2023 13:51

DOP-3903-b remove extra ternary ops

fe20fc9

Merge branch 'master' into DOP-3903-b

201ee3d

DOP-3903-b cleaned up merge

340bb23

DOP-3903-b unit tests added

5932cd2

DOP-3903-b unit tests

811ce88

anabellabuckvar requested a review from mmeigs October 3, 2023 21:42

branberry approved these changes Oct 4, 2023

View reviewed changes

Merge branch 'master' into DOP-3903-b

c2ceae3

mmeigs reviewed Oct 5, 2023

View reviewed changes

DOP-3903-b refactored hidechange functions

a92ad25

anabellabuckvar and others added 2 commits October 6, 2023 13:07

DOP-3903-b redid tests

44e3090

Merge branch 'master' into DOP-3903-b

d0d54d5

anabellabuckvar requested a review from mmeigs October 6, 2023 17:52

mmeigs reviewed Oct 6, 2023

View reviewed changes

anabellabuckvar added 2 commits October 6, 2023 16:34

DOP-3903-b moved function calls

9ebb673

DOP-3903-b removing, adding comments

2f8a825

anabellabuckvar requested a review from mmeigs October 6, 2023 21:10

Merge branch 'master' into DOP-3903-b

f9a48f2

mmeigs reviewed Oct 9, 2023

View reviewed changes

Merge branch 'master' into DOP-3903-b

d5ad525

mmeigs approved these changes Oct 10, 2023

View reviewed changes

anabellabuckvar merged commit b6308b3 into master Oct 10, 2023
2 checks passed

anabellabuckvar deleted the DOP-3903-b branch October 10, 2023 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOP-3903: API Changelog Support to omit entries #919

DOP-3903: API Changelog Support to omit entries #919

anabellabuckvar commented Sep 26, 2023 •

edited

Loading

caesarbell left a comment

caesarbell commented Sep 28, 2023

branberry left a comment •

edited

Loading

branberry Oct 3, 2023 •

edited

Loading

branberry Oct 3, 2023

branberry Oct 3, 2023

branberry Oct 3, 2023

branberry Oct 3, 2023

branberry Oct 3, 2023

mmeigs left a comment

mmeigs Oct 3, 2023

mmeigs Oct 3, 2023

mmeigs Oct 3, 2023

branberry left a comment

mmeigs Oct 3, 2023

mmeigs Oct 3, 2023

mmeigs Oct 5, 2023

mmeigs Oct 5, 2023

anabellabuckvar Oct 6, 2023

mmeigs Oct 5, 2023

anabellabuckvar Oct 6, 2023

mmeigs left a comment

mmeigs Oct 6, 2023

mmeigs Oct 6, 2023

mmeigs Oct 6, 2023

mmeigs left a comment

anabellabuckvar commented Oct 9, 2023 •

edited

Loading

mmeigs left a comment

DOP-3903: API Changelog Support to omit entries #919

DOP-3903: API Changelog Support to omit entries #919

Conversation

anabellabuckvar commented Sep 26, 2023 • edited Loading

Stories/Links:

Current Behavior:

Staging Links:

Notes:

caesarbell left a comment

Choose a reason for hiding this comment

caesarbell commented Sep 28, 2023

branberry left a comment • edited Loading

Choose a reason for hiding this comment

branberry Oct 3, 2023 • edited Loading

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

mmeigs 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

branberry 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

mmeigs 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

mmeigs left a comment

Choose a reason for hiding this comment

anabellabuckvar commented Oct 9, 2023 • edited Loading

mmeigs left a comment

Choose a reason for hiding this comment

anabellabuckvar commented Sep 26, 2023 •

edited

Loading

branberry left a comment •

edited

Loading

branberry Oct 3, 2023 •

edited

Loading

anabellabuckvar commented Oct 9, 2023 •

edited

Loading