search

5am-code / laravel-notion-api

Effortless Notion integrations with Laravel
https://notionforlaravel.com
MIT License
410 stars 50 forks source link

add quote block type #161

Closed Gummibeer closed 1 year ago

what-the-diff[bot] commented 1 year ago

PR Summary

johguentner commented 1 year ago

Hey @Gummibeer, Thank you for submitting your PR! Would be awesome, if you could add tests for your implementation.

I just looked into it, and it shouldn't be too much to do.

It would be totally sufficient, if you add it within these two test-methods (so you don't have to create a new test-file):

The first one is pretty simple, since it can pretty much be copy-pasted from above

For the second one it's almost the same, but you should add the json-structure for the new block to the snapshot: https://github.com/5am-code/laravel-notion-api/blob/main/tests/stubs/endpoints/blocks/response_specific_supported_blocks_200.json

If you have any questions, feel free to reach out! :)

I'll make sure to add it to the docs when it's released.

I'll make sure to push this as 1.1.1 or even 1.2.0 with other new features in the near future!

Gummibeer commented 1 year ago

@johguentner I have looked into the test but it looks like it uses a fixture based on a notion doc you own somewhere!? For sure I could rebuild the partial response for the quote block and add it there. But wanted to check if that's the way to go or if you would add it in notion and update the fixture first?

johguentner commented 1 year ago

@johguentner I have looked into the test but it looks like it uses a fixture based on a notion doc you own somewhere!? For sure I could rebuild the partial response for the quote block and add it there. But wanted to check if that's the way to go or if you would add it in notion and update the fixture first?

Most of the current tests (including the Block endpoint) are based on self-written (or manually generated) snapshots. So there is no real Notion Workspace, which is corresponding to the package.

If nothing is wrong, you should be able to run ./vendor/bin/pest without an issue, since no active API call is being made. (If there is a problem, it would be great if you could provide the error-message, since this is - of course - not intended).

This solution is not optimal, but alright for the moment (a better solution is in progress and described below). So please feel free to basically copy-past the testing-structure in the test-methods and add the structure within the JSON snapshots. If anything is weird or does not make sense, feel free to reach out.

For new features (since v1.1.0) we have created a new testing-system, which is based on a Notion workspace and will automatically create local snapshots. We do that to avoid API calls (unless forced) for speed during tests and providing tests when running outside our environment. Currently we have not fully figured out, how we can provide reliable testing-ground for external contributors when developing new features. As soon, as we have figured that out, we will replace all tests with the new system, since it is much more reliable, because it better represents the Notion structure and can renew automatically (at least for the JSON snapshots) if there are breaking changes.

Gummibeer commented 1 year ago

For new features (since v1.1.0) we have created a new testing-system, which is based on a Notion workspace and will automatically create local snapshots. We do that to avoid API calls (unless forced) for speed during tests and providing tests when running outside our environment. Currently we have not fully figured out, how we can provide reliable testing-ground for external contributors when developing new features. As soon, as we have figured that out, we will replace all tests with the new system, since it is much more reliable, because it better represents the Notion structure and can renew automatically (at least for the JSON snapshots) if there are breaking changes.

That was what I thought is already the case.^^ Have created the fixture manually now.

The Saloon package by @Sammyjo20 has a record response feature: https://docs.saloon.dev/testing/recording-requests You could implement such a system and add a GitHub Action with your secrets that will generate the correct fixtures automatically. So yes, locally the tests would be theoretical but besides totally new features should be possible. And after first commit GH would create/update the fixture so that I could even pull down the fresh fixture and properly adjust my tests if needed.

I mean for this PR it would still require you to add the new Quote block manually as it's a new supported type. Or you would just add all possible blocks to one page and the implemented check tests would just have to pick the correct index for testing.

johguentner commented 1 year ago

That was what I thought is already the case.^^ Have created the fixture manually now.

The Saloon package by @Sammyjo20 has a record response feature: https://docs.saloon.dev/testing/recording-requests You could implement such a system and add a GitHub Action with your secrets that will generate the correct fixtures automatically. So yes, locally the tests would be theoretical but besides totally new features should be possible. And after first commit GH would create/update the fixture so that I could even pull down the fresh fixture and properly adjust my tests if needed.

I mean for this PR it would still require you to add the new Quote block manually as it's a new supported type. Or you would just add all possible blocks to one page and the implemented check tests would just have to pick the correct index for testing.

That's a really good suggestion! I never though about using Github Actions regarding the generation of these snapshots. This would even work with what we have planned within v1.1.0.

The only objection I'd have about this, is that it would allow people to modify the according workspace with possible malicious intents (not likely, however possible), but I'm sure there is a good solution for this - like creating a token, which has only access to a specific page, which is specifically designed for the implementation of external features.

I'll talk with @mechelon about this idea.

Since we still have not pushed v1.1.0 and your tests look perfect, I'll add this feature to the current release. Should be out in the next few days.

Gummibeer commented 1 year ago

You could use a readonly token for that GH Action. In case a write action needs a new fixture you could still checkout the code locally or even use a PR action which, so far I know, has to be manually allowed by a team member if the PR is created by an external contributor.

At least I have to allow the test actions for external contributors in my projects.