Repository navigation
Add option for FairData-based media store - #801
Conversation
✅ Deploy Preview for marronage-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for pss-scavenger-hunt ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for padp-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for relnet-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
I don't see this anywhere in published docs, but it looks like the MediaStore interface actually has a read-only option already called isStatic:
/**
* Indicates that uploads and deletions are not supported
*
* @default false
*/
isStatic?: boolean;I wonder if you just add isStatic = true if that would eliminate the need for media-ui.ts?
I could see it being a bit brittle to rely on an undocumented option—maybe we should add a unit test to make sure that this option is still supported and does what we expect, so we can catch it when we upgrade Tina in the future. To me this seems less brittle than removing buttons by label.
Good catch! Yeah I dislike the "try to find the right buttons by searching the DOM structure in a somewhat ad-hoc way and disable them" approach haha, but did not realize that the |
| }); | ||
|
|
||
| test('Media manager hides folder/upload/delete controls for the FairData store', async ({ page }) => { | ||
| test.skip(process.env.TINA_PUBLIC_USE_FD_MEDIA !== 'true', 'FairData media store is not enabled'); |
There was a problem hiding this comment.
Re: this test.skip rule, I remember generally we were trying to avoid allowing the dev's local .env to change test results. Should we set this true here instead?
core-data-places/playwright.rbac.config.ts
Lines 69 to 84 in 7198c39
There was a problem hiding this comment.
Now that this is hard coded in the RBAC playwright config, should we remove the skip here, and the TINA_PUBLIC_USE_FD_MEDIA: true in the github workflow?
| }; | ||
| }); | ||
|
|
||
| const totalPages = Number(response.list?.pages) || page; |
There was a problem hiding this comment.
Claude flagged that because this is running in the browser rather than server-side, and using MediaContentsService from @backend/api/coreData/mediaContents, that means it's using the JSON that omits list, so response.list will always be undefined here. It recommends adding the list field here, which is the server-side handling for the API response from FairData (which will have list) that actually is used to build the JSON:
core-data-places/src/services/coreData/base.ts
Lines 36 to 50 in cc4a10a
I think this should only hit the else block in that function here since media_contents has no content-collection loader, afaik, and so it could just store list from the fetchAll response on a local variable and return it alongside [this.name]: records if it's defined.
This will add a list field to every /api/<model>/index.json response, but I think that's reasonable/useful.
There was a problem hiding this comment.
Oh good catch, I'm not sure I fully realized that the /api routes don't return that data currently! I tested that it could still fetch the media with this paginated setup but I suppose it was just returning the first 100 and then stopping. 🤔
| }); | ||
|
|
||
| test('Media manager hides folder/upload/delete controls for the FairData store', async ({ page }) => { | ||
| test.skip(process.env.TINA_PUBLIC_USE_FD_MEDIA !== 'true', 'FairData media store is not enabled'); |
There was a problem hiding this comment.
Now that this is hard coded in the RBAC playwright config, should we remove the skip here, and the TINA_PUBLIC_USE_FD_MEDIA: true in the github workflow?
| const firstMediaItem = page.locator('ul li button').first(); | ||
| await expect(firstMediaItem).toBeVisible(); | ||
| await firstMediaItem.click(); | ||
| await expect(page.getByRole('button', { name: 'Delete', exact: true })).toHaveCount(0); |
There was a problem hiding this comment.
I think this will be true (no delete button) between the click and when the item's detail panel opens, so it might not exactly be checking the contents of the panel. Maybe best if we wait for something we know will be inside of the panel first? e.g.
| await expect(page.getByRole('button', { name: 'Delete', exact: true })).toHaveCount(0); | |
| await expect(page.getByRole('button', { name: 'Insert', exact: true })).toBeVisible(); | |
| await expect(page.getByRole('button', { name: 'Delete', exact: true })).toHaveCount(0); |
|
Also just noticed the test failure in CI. It looks like it might be selecting the edit mode button? Might need to call |
In this PR
Adds the ability to configure (via environment variable, for now) the Tina media store to connect directly to the associated FairData project(s) rather than a separate S3 backend. This option makes the media store read-only on the Tina side -- all upload, deletion etc. is done via FairData. As such, I also added some extra UI tinkering to disable the
Upload,New Folder, andDeletebuttons when using this option. (This disabling is cosmetic, but the custom media store has no-op methods for deletion and upload in any case.)This works in tandem with the more feature-rich FairData record (and media) MDX inserts, allowing FairData media records to be used as thumbnails, inserted into rich text fields just as plain old images rather than IIIF viewers, and used in the layout of custom pages.