-
Notifications
You must be signed in to change notification settings - Fork 5
EES-5934 Fixing Admin Public API Data Set Changelog page #5705
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
Conversation
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.
Nice! And neat too!
Please could you grab someone to check the front end stuff as I don't understand that stuff. Once they're ready, I'm happy for it to be approved.
@@ -73,6 +75,31 @@ public async Task<Either<ActionResult, PaginatedListViewModel<DataSetLiveVersion | |||
}); | |||
} | |||
|
|||
public async Task<Either<ActionResult, DataSetVersionInfoViewModel>> GetDataSetVersion( |
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.
You don't want to call this GetDataSetVersionInfo
?
@@ -267,7 +280,7 @@ private async Task<DataSetVersionSummaryViewModel> MapDraftVersionSummary( | |||
}; | |||
} | |||
|
|||
private async Task<Either<ActionResult, DataSetVersion>> GetDataSetVersion( | |||
private async Task<Either<ActionResult, DataSetVersion>> GetVersion( |
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 don't think this naming makes sense as it isn't returning a version.
const dataSetVersionIsDraft = (dataSetVersionStatus: DataSetVersionStatus) => | ||
dataSetVersionStatus === 'Draft'; |
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.
NIT: seems a bit verbose, where we set isDraft this is fine
const isDraft = dataSetVersion?.status === 'Draft'
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.
LGTM!
Previously, when an admin visited the Public API version history page for a particular API data set, and clicked to view the changelog for versions
1.1
and above, they were sometimes displayed with the incorrect version number and version 'type' (Major
/Minor
).This was because the frontend had some logic which only ever showed the version number and type for either the latest live version of the data set, or the current draft version, depending on whether or not the version you selected is the draft one or not. It didn't display the details for the version you have specifically requested (caveat - it DID do this sometimes, but only by coincidence).
For example, previously, the changelog page for the following versions showed:
2.0
-> Displayed2.0
Major
-> CORRECT (Coincidentally)1.2
-> Displayed2.0
Major
-> INCORRECT1.1
-> Displayed2.0
Major
-> INCORRECT1.0
-> Can't access the changelog page for the first versionThis PR makes the changes necessary for the page to retrieve the data it needs specific to the requested data set version. We have introduced a new Admin API
GET
endpoint for this, for the Data Set Version.