-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-3903: API Changelog Support to omit entries #919
Changes from 8 commits
cd1b530
73184b4
00dedcb
6f8af87
6f341b2
3d90d72
7ef99b6
7bac35e
fe20fc9
201ee3d
340bb23
5932cd2
811ce88
c2ceae3
a92ad25
44e3090
d0d54d5
9ebb673
2f8a825
f9a48f2
d5ad525
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,7 +82,37 @@ const fetchChangelogData = async (runId, versions, s3Prefix) => { | |
try { | ||
/* Using metadata runId, fetch OpenAPI Changelog full change list */ | ||
const changelogResp = await fetch(`${s3Prefix}/${runId}/changelog.json`); | ||
const changelog = await changelogResp.json(); | ||
const unfilteredChangelog = await changelogResp.json(); | ||
|
||
const hideChanges = (changelog) => { | ||
const versionUpdate = (version) => { | ||
const updatedVersion = { ...version }; | ||
if (version?.changes) { | ||
updatedVersion.changes = version.changes.filter((change) => !change.hideFromChangelog); | ||
} | ||
return updatedVersion; | ||
}; | ||
|
||
//pathUpdate takes the array of versions from the specific path passed in and takes each version and runs versionUpdate on it | ||
const pathUpdate = (path) => { | ||
const updatedPath = { ...path, versions: path.versions.map(versionUpdate) }; | ||
updatedPath.versions = updatedPath.versions.filter((version) => version?.changes?.length); | ||
return updatedPath; | ||
}; | ||
|
||
//dateUpdate takes the array of paths from the specific date section passed in and takes each path and runs pathUpdate on it | ||
const dateUpdate = (dateSection) => { | ||
const updatedDateSection = { ...dateSection, paths: dateSection.paths.map(pathUpdate) }; | ||
updatedDateSection.paths = updatedDateSection.paths.filter((path) => path?.versions?.length); | ||
return updatedDateSection; | ||
}; | ||
|
||
//changelog is the json file with everything in it. Map at this level takes each date section and runs dateUpdate on it | ||
const updatedChangelog = changelog.map(dateUpdate); | ||
return updatedChangelog.filter((dateSection) => dateSection?.paths?.length); | ||
}; | ||
|
||
const changelog = hideChanges(unfilteredChangelog); | ||
|
||
/* Aggregate all Resources in changelog for frontend filter */ | ||
const resourcesListSet = new Set(); | ||
|
@@ -95,7 +125,23 @@ const fetchChangelogData = async (runId, versions, s3Prefix) => { | |
const mostRecentResourceVersions = versions.slice(-2); | ||
const mostRecentDiffLabel = mostRecentResourceVersions.join('_'); | ||
const mostRecentDiffResp = await fetch(`${s3Prefix}/${runId}/${mostRecentDiffLabel}.json`); | ||
const mostRecentDiffData = await mostRecentDiffResp.json(); | ||
const mostRecentDiffDataUnfiltered = await mostRecentDiffResp.json(); | ||
|
||
//nested filtering of Diff changes with hideFromChangelog | ||
const hideDiffChanges = (diffData) => { | ||
const pathUpdate = (path) => { | ||
const updatedPath = { ...path }; | ||
if (path?.changes) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't need an optional chain here! |
||
updatedPath.changes = path.changes.filter((change) => !change.hideFromChangelog); | ||
} | ||
return updatedPath; | ||
}; | ||
|
||
const updatedDiffData = diffData.map(pathUpdate); | ||
return updatedDiffData.filter((path) => path?.changes?.length); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, no need for the optional chain on path, but yes, keep the one on changes! |
||
}; | ||
|
||
const mostRecentDiffData = hideDiffChanges(mostRecentDiffDataUnfiltered); | ||
|
||
return { | ||
changelog, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,20 @@ import { fetchOADiff } from '../../../utils/realm'; | |
import useChangelogData from '../../../utils/use-changelog-data'; | ||
import { getDiffRequestFormat } from './getDiffRequestFormat'; | ||
|
||
//nested filtering of Diff changes with hideFromChangelog | ||
const hideDiffChanges = (diffData) => { | ||
const pathUpdate = (path) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logic looks like it's the same from the filtering of the changelog in |
||
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); | ||
}; | ||
|
||
export const useFetchDiff = (resourceVersionOne, resourceVersionTwo, setIsLoading, setToastOpen, snootyEnv) => { | ||
const { index = {}, mostRecentDiff = {} } = useChangelogData(); | ||
const [diff, setDiff] = useState([]); | ||
|
@@ -20,7 +34,8 @@ export const useFetchDiff = (resourceVersionOne, resourceVersionTwo, setIsLoadin | |
setIsLoading(true); | ||
fetchOADiff(index.runId, fromAndToDiffLabel, snootyEnv) | ||
.then((response) => { | ||
setDiff(response); | ||
const filteredDiff = hideDiffChanges(response); | ||
setDiff(filteredDiff); | ||
setIsLoading(false); | ||
}) | ||
.catch((err) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
would become
and then return the
updatedPath
rather than thepath
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.