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

Added Default Templates for Chapters #4750

Merged

Conversation

Man-in-Black
Copy link
Contributor

I have extended the new feature of default templates for books to chapters.
This makes it possible to specify default templates within individual chapters of a book so that they are automatically selected when a new page is created.
The functionality is exactly the same as with books.
Here is an excerpt from my bookstack (in german):
image
Behind the scenes I've made changes to the controller, the repos, the models and the trash can.
There is also a migration file for the database to alter the table for the chapters so that you can set the default_template_id.

Copy link
Member

@ssddanbrown ssddanbrown left a comment

Choose a reason for hiding this comment

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

Thanks @Man-in-Black for this PR, it's looking good and generally all seems to work apart from one minor condition.
I've added a set of notes on the code to work through.

In addition to these, there are a couple of other areas that would need consideration:

  • This functionality would need testing coverage. The tests for book templates can be found here. The tests would be similar, with crossover in testing, so that existing test file could just be changed to DefaultTemplateTest, with book added to the existing test case names if not already there.
  • The REST API needs to be considered, to allow templates to be set/updated for chapters via the API. The API examples, and tests, would then need to be updated to suit.

If you're unsure of anything written, need help, or would instead prefer me to take on the requested work, just let me know.

app/Entities/Repos/ChapterRepo.php Show resolved Hide resolved
app/Entities/Repos/PageRepo.php Show resolved Hide resolved
app/Entities/Controllers/PageController.php Outdated Show resolved Hide resolved
app/Entities/Tools/TrashCan.php Outdated Show resolved Hide resolved
resources/views/chapters/parts/form.blade.php Outdated Show resolved Hide resolved
@Man-in-Black
Copy link
Contributor Author

Man-in-Black commented Jan 17, 2024

Thank you Dan for your comments, if it's okay for you I would to try to fix the parts you mentioned.
But if I got stucked somewhere I would be glad if you can give me some advise, as these are my first "bigger" steps into Laravel.

@ssddanbrown
Copy link
Member

Sure, sounds good! Happy to help and advise where needed.
If you'd prefer help via Discord chat instead of here feel free to create a thread in the help-and-support channel.

remove duplicate call of $page->forceDelete();
@Man-in-Black
Copy link
Contributor Author

Hi @ssddanbrown

I'm very happy that you accepted my first "bigger" PR :-)
And I'm also happy that it generally looks good.

I'm trying to get into your comments and suggestions, but to be honest I'm struggling a bit at this point.

  • I think I can provide the test cases, but unfortunately I don't know how to run such a test. Maybe you can give me some guidance on how to execute a test case.
  • For the part with the API I just found 2 lines to be added in app/Entities/Controllers/ChapterApiController.php, but this seems to easy for me.
    And also here, how could I start a test case?

The most parts I've done comes from my former PHP knowledge, so it was doable for me. But some years passed since then and after I discovered your project I've started to get back into it, but I think it needs some more time.
I would love to learn something with this PR, but don't be mad at me if I can't resolve all your comments.

I hope this is okay for you and if I'm not able to resolve everything I would be happy if you can take on the work, I would then start to build up my knowledge on your changes.

@ssddanbrown
Copy link
Member

Maybe you can give me some guidance on how to execute a test case.

Sure, I have specific guidance covering exactly this on this page here. Let me know if there's something missing from that, or if you have trouble with any of the details there.

For the part with the API I just found 2 lines to be added in app/Entities/Controllers/ChapterApiController.php, but this seems to easy for me.

That might be all that's needed to be honest, most of the logic is handled in the repos so we can share it across the API and normal app code, we just have to make sure those properties are accepted through validation.

In regard to testing the API changes, you can see my changes to BooksApiTest in the changes for this PR. I didn't add any specific API tests for this, but just updated existing tests to also include a default template.

I would love to learn something with this PR, but don't be mad at me if I can't resolve all your comments.
I hope this is okay for you and if I'm not able to resolve everything I would be happy if you can take on the work, I would then start to build up my knowledge on your changes.

I'm not mad at all, happy to take the time where possible to get others involved and to help folks learn.
Just feel free to let me know if you get to a blocking point, need help, or want me to take on parts.

@Man-in-Black
Copy link
Contributor Author

Man-in-Black commented Jan 29, 2024

Sure, I have specific guidance covering exactly this on this page here. Let me know if there's something missing from that, or if you have trouble with any of the details there.

Thanks for the guide. I followed it but I can't get it to run, I always get the following error:

root@bookstack:/var/www/bookstack/tests# composer test tests/HomepageTest.php
No composer.json in current directory, changing working directory to /var/www/bookstack
Do not run Composer as root/super user! See https://getcomposer.org/root for details
Continue as root/super user [yes]?
> phpunit 'tests/HomepageTest.php'
PHP Fatal error:  Uncaught Error: Class "Tests\TestCase" not found in /var/www/bookstack/tests/HomepageTest.php:8
Stack trace:
#0 /usr/share/php/PHPUnit/Util/FileLoader.php(65): include_once()
#1 /usr/share/php/PHPUnit/Util/FileLoader.php(49): PHPUnit\Util\FileLoader::load()
#2 /usr/share/php/PHPUnit/Runner/StandardTestSuiteLoader.php(43): PHPUnit\Util\FileLoader::checkAndLoad()
#3 /usr/share/php/PHPUnit/Runner/BaseTestRunner.php(146): PHPUnit\Runner\StandardTestSuiteLoader->load()
#4 /usr/share/php/PHPUnit/Runner/BaseTestRunner.php(112): PHPUnit\Runner\BaseTestRunner->loadSuiteClass()
#5 /usr/share/php/PHPUnit/TextUI/Command.php(120): PHPUnit\Runner\BaseTestRunner->getTest()
#6 /usr/share/php/PHPUnit/TextUI/Command.php(96): PHPUnit\TextUI\Command->run()
#7 /usr/bin/phpunit(73): PHPUnit\TextUI\Command::main()
#8 {main}

Next PHPUnit\TextUI\RuntimeException: Class "Tests\TestCase" not found in /usr/share/php/PHPUnit/TextUI/Command.php:98
Stack trace:
#0 /usr/bin/phpunit(73): PHPUnit\TextUI\Command::main()
#1 {main}
  thrown in /usr/share/php/PHPUnit/TextUI/Command.php on line 98
Script phpunit handling the test event returned with error code 255

That might be all that's needed to be honest, most of the logic is handled in the repos so we can share it across the API and normal app code, we just have to make sure those properties are accepted through validation.

Ok I've added the two lines that seemed right to me.

I'm not mad at all, happy to take the time where possible to get others involved and to help folks learn. Just feel free to let me know if you get to a blocking point, need help, or want me to take on parts.

I'm very glad about that. I just commented on some of your comments to see if you can adopt them.
I have processed all other comments to the best of my knowledge.

@ssddanbrown
Copy link
Member

I always get the following error:

Ah, you may still have PHP (composer) dependancies installed for production.
Try running composer install, you should see a few extra things get installed.

By default, we state to run composer install --no-dev which avoids autoloading test code, or downloading test/development requirements.

@Man-in-Black
Copy link
Contributor Author

Thanks for the clarification. It realy installed some new packages.
But after I installed them my bookstack instance stopped working with an error 500.
Maybe because of this I'm getting a bunch or errors while running the tests.

I think at the moment I've reached my limits in this regards and I do need to learn more about it before writing a valid test case.

ssddanbrown added a commit that referenced this pull request Feb 1, 2024
Also applied minor tweaks to some wording and logic.

During review of #4750
ssddanbrown added a commit that referenced this pull request Feb 1, 2024
- Updated existing book tests to be generic to all default templates,
  and updated with chapter testing.
- Extracted repeated logic in the Book/Chapter repos to be shared in the
  BaseRepo.

Review of #4750
@ssddanbrown ssddanbrown merged commit 4a8f702 into BookStackApp:development Feb 1, 2024
5 of 11 checks passed
@ssddanbrown
Copy link
Member

ssddanbrown commented Feb 1, 2024

Thanks again @Man-in-Black for offering this and progressing it upon my requests.
I've now merged it in after taking on the remaining parts you wanted me to take on, or you reached your limit with.

My further changes can be seen in 4137cf9 and 43a72fb.
This will be part of the next feature release.

@Man-in-Black
Copy link
Contributor Author

Man-in-Black commented Feb 2, 2024

Thanks @ssddanbrown for taking on this.
After I reviewed your changes it now seems quite obvious to me what you have done.
Thanks again and I'm very happy that I got the opportunity to contribute to this great product which everyone I know is loving and is also using it after my recommendation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants