Skip to content

add quote block type #161

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

Merged
merged 4 commits into from
Jun 20, 2023
Merged

add quote block type #161

merged 4 commits into from
Jun 20, 2023

Conversation

Gummibeer
Copy link
Contributor

No description provided.

@what-the-diff
Copy link

what-the-diff bot commented Jun 19, 2023

PR Summary

  • Added support for 'quote' block type
    Enhanced functionality by introducing support for 'quote' blocks in Block.php.
  • Implemented 'quote' block type in Quote.php
    Created a new file, Quote.php, containing the complete implementation of the 'quote' block type.

@johguentner johguentner added this to the 🍩 1.2.0 milestone Jun 19, 2023
@johguentner johguentner added enhancement New feature or request tests-required Tests for this PR are required labels Jun 19, 2023
@johguentner
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

johguentner commented Jun 19, 2023

@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
Copy link
Contributor Author

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 johguentner removed the tests-required Tests for this PR are required label Jun 20, 2023
@johguentner johguentner changed the base branch from main to dev June 20, 2023 11:53
@johguentner johguentner modified the milestones: 🍩 1.2.0, 🍵 v1.1.0 Jun 20, 2023
@johguentner
Copy link
Member

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.

@johguentner johguentner merged commit 6b31bcd into 5am-code:dev Jun 20, 2023
@Gummibeer
Copy link
Contributor Author

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.

@Gummibeer Gummibeer deleted the add-quote-block branch June 20, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants