Skip to content
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

API Remove GraphQL #1477

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Jul 22, 2024

@emteknetnz emteknetnz marked this pull request as draft July 22, 2024 05:02
@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch 5 times, most recently from cab8f2e to c5ed1bc Compare July 23, 2024 06:53
@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch 13 times, most recently from 9e27562 to 2afd9a1 Compare July 29, 2024 22:30
@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch 7 times, most recently from f418bce to 08efa29 Compare July 30, 2024 06:14
@emteknetnz emteknetnz changed the title API Standardise endpoints API Remove GraphQL Jul 30, 2024
@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch 6 times, most recently from 7dd90f7 to 9ac18a4 Compare August 19, 2024 05:43
@emteknetnz emteknetnz marked this pull request as ready for review August 19, 2024 22:13
_config/config.yml Outdated Show resolved Hide resolved
code/Controller/AssetAdmin.php Show resolved Hide resolved
code/Controller/AssetAdmin.php Show resolved Hide resolved
code/Controller/AssetAdmin.php Outdated Show resolved Hide resolved
code/Controller/AssetAdmin.php Outdated Show resolved Hide resolved
tests/behat/features/manage-files.feature Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch 5 times, most recently from 3f950c5 to ed5efa5 Compare August 22, 2024 00:41
_config/config.yml Show resolved Hide resolved
client/src/components/UploadField/UploadField.js Outdated Show resolved Hide resolved
client/src/containers/AssetAdmin/AssetAdmin.js Outdated Show resolved Hide resolved
client/src/containers/Editor/Editor.js Show resolved Hide resolved
client/src/containers/Gallery/Gallery.js Show resolved Hide resolved
code/Controller/AssetAdmin.php Outdated Show resolved Hide resolved
code/Controller/AssetAdmin.php Show resolved Hide resolved
code/Controller/AssetAdmin.php Show resolved Hide resolved
code/Controller/AssetAdmin.php Show resolved Hide resolved
code/Controller/AssetAdminOpen.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch from ed5efa5 to 096335d Compare August 22, 2024 08:18
@GuySartorelli
Copy link
Member

Few problems found while testing edge cases:

  1. It seems like list view in the asset admin isn't using thumbnails. If I delete the thumbnail variants and then view the list view, I still see the image. If I right click on it and view image in new tab, it's the original full resolution image.
  2. If AssetAdminOpen::apiRead() throws a json error, the asset admin doesn't load correctly and I see this in the dev console (yes it does just end abruptly - there's no more info other than a js stack trace):
    Uncaught (in promise) Error: 
    
  3. if AssetAdmin::apiReadDescendantCounts() throws a json error, the confirm deletion modal is stuck loading with the same error as above in the dev console

I stopped checking at that point - it seems likely that all of the endpoints suffer from similar problems.
I didn't check elemental, but suspect it would probably have the same type of problems.

@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 25, 2024

It seems like list view in the asset admin isn't using thumbnails. If I delete the thumbnail variants and then view the list view, I still see the image. If I right click on it and view image in new tab, it's the original full resolution image.

Replicated this on 5.2, so looks like it's an existing issue

@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch 4 times, most recently from fec2587 to a55c7d1 Compare August 28, 2024 00:22
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new json error handling is great!

Found one problem when testing this time around.

Cannot cancel deleting a file

  1. Upload a file.
  2. Select the file.
  3. Click the ... button
  4. Click delete
  5. Try to cancel the modal (it won't go away, and there's no errors in the console, no network requests, nothing)

The fact this wasn't caught by behat means we are missing a test scenario for this.

@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch from a55c7d1 to a7666b3 Compare August 29, 2024 04:23
@emteknetnz
Copy link
Member Author

Have fixed and added behat tests for cancelling and closing the modal

@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch from a7666b3 to 49bb344 Compare August 29, 2024 06:44
@emteknetnz emteknetnz force-pushed the pulls/3/standardise-endpoints branch from 49bb344 to c34ad4e Compare August 30, 2024 00:06
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, works as expected locally.

@GuySartorelli GuySartorelli merged commit bf843a9 into silverstripe:3 Sep 2, 2024
5 of 14 checks passed
@GuySartorelli GuySartorelli deleted the pulls/3/standardise-endpoints branch September 2, 2024 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants