Upgrade pdc sdk #881
Upgrade pdc sdk #881
Conversation
✅ Deploy Preview for philanthropy-data-commons-viewer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@hminsky2002 I added the above deploy preview address to the valid redirect URIs for the client associated with the front end, so now login should work there. And when looking at the preview, I see the Changemakers and the Proposals. Nice. |
This PR updates the @pdc-sdk to the latest version to use the new ChangeMaker type, and renames all components, routes, and variables related to Organizations to Changemaker. Notably, sample data that mocks base-field data has been kept the same, even if it does refer to organizations.
This commit adds a method for providing a source for bulkuploads on the
front-end. Using the new useSources hook, it pulls a (set) number of
sources, then filters for the source using the determined system source
shortcode ('pdc'). Notably, this approach is limited to the scenarios
where the service has fewer than the default count of sources, but since
there is currently no way of querying sources on shortcode, this is the
preferred approach. See pdc-service issue 1295
(PhilanthropyDataCommons/service#1295)
hminsky2002
force-pushed
the
836-upgrade-pdc-sdk
branch
from
189c82a
to
10eec3b
Compare
There was a problem hiding this comment.
I was able to see the app in action at the deploy preview. I did not try to run it locally, however. The changes all appear expected to me.
Since the app is broken until this gets merged, I am smashing "Approve" even though I know near-zero React. If you'd like to wait for others feedback, I cannot disagree with that decision either.
My simple way of thinking is that by merging this PR, if something breaks it won't be significantly more broken than it is now, and we have an opportunity to unbreak it with a merge.
| sourceId: systemSource?.id, | ||
| }); | ||
| } else { | ||
| throw new Error('No System Source Available'); |
There was a problem hiding this comment.
Good call throwing an Error here. I wonder how it surfaces and if the message could be more explicit, like No System Source with short code = yada found at [url used].
| * @param {string} systemSourceShortCode the shortcode for the system source | ||
| * @return {Source | undefined} the system source, or undefined if it does not exist | ||
| */ | ||
| const getSystemSource = (entries: Source[], systemSourceShortcode: string) => |
There was a problem hiding this comment.
Unfortunate that this needs to be done but a clean and concise implementation given that it needs to be done!
There was a problem hiding this comment.
Looking good! Thanks for working on this, @hminsky2002.
I believe this covers what we discussed during my office hour.
| @@ -6,7 +6,7 @@ | |||
| "@axa-fr/react-oidc": "^7.22.32", | |||
| "@fontsource/source-sans-pro": "^5.1.0", | |||
| "@heroicons/react": "^2.1.3", | |||
| "@pdc/sdk": "^0.11.1", | |||
| "@pdc/sdk": "^0.16.1", | |||
There was a problem hiding this comment.
Right now, this upgrade happens in the first commit, and therefore the first commit does not build.
Instead, let's gradually bump the SDK version along with the changes necessary for that specific version of the SDK. I believe, in order, they are: handle timestamp strings, include a source id with bulk uploads, and rename organizations to changemakers.
There was a problem hiding this comment.
@jasonaowen Egads! I've hit an unfortunate issue here -- the SourceBundle, which is needed for the source handling, was missed in the openapi spec and I had to add it back in (PhilanthropyDataCommons/service#1281) at a later date. As such, an older version of the pdc cannot be used for the sourceId commit without also refactoring many organizations to changemakers. Thoughts on either merging the two commits? That seems messy but then again, the changes are separate enough that they might still be discnerable
| '/sources', | ||
| new URLSearchParams({ | ||
| _page: page, | ||
| _count: count, |
There was a problem hiding this comment.
This works, but the API docs do not list the _page and _count parameters. New issue opened: PhilanthropyDataCommons/service#1313
| @@ -31,6 +38,7 @@ interface BulkUploaderProps { | |||
| onBulkUpload: () => void; | |||
| } | |||
| const BulkUploader = ({ onBulkUpload }: BulkUploaderProps) => { | |||
| const [sources] = useSources(SOURCES_DEFAULT_PAGE, SOURCES_DEFAULT_COUNT); | |||
There was a problem hiding this comment.
The problem that @hminsky2002 called out with this is that if the system source does not show up in this call, we error down below.
This is exasperated by two factors: using a synthetic key so that we need to do this at all, and that the /sources endpoint returns the most recent sources (sorted by id descending) rather than the earliest sources, as the system source should always be the first source.
The change that introduced the system source, PhilanthropyDataCommons/service#1116, created the table and inserted the system source row in the same migration, and the id is GENERATED ALWAYS AS IDENTITY, so the system source ID should always be 1 and cannot be UPDATEd. Maybe it's worth hardcoding that here? cc @slifty
pdc=*> update sources set id = 2;
ERROR: column "id" can only be updated to DEFAULT
DETAIL: Column "id" is an identity column defined as GENERATED ALWAYS.
This PR upgrades the sdk to the latest version, and refactors the codebase to reflect two major updates: the change in name from the entity 'organization' to 'changemaker', and the requirement for a source id to be referenced when creating a new bulkupload. Notably, the current approach to providing a source is limited to a constant number, wherein we fetch the first 1000 sources in the database and filter only on that. See service issue 1295.
Closes #836