luyadev / luya-module-cms

The LUYA CMS module provides a full functional CMS for adding contents based on blocks.
https://luya.io
MIT License
33 stars 46 forks source link

Fixed navItem relation for inactive page versions #415

Closed hbugdoll closed 8 months ago

hbugdoll commented 9 months ago

What are you changing/introducing

What is the reason for changing/introducing

QA

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues Closes #414
nadar commented 9 months ago

nav_item_id does not exist in NavItemModule and NavItemRedirect it only exists in NavItemPage, so this will break things.

hbugdoll commented 9 months ago

Many thanks for this important hint!

One last try: Would it be acceptable for you to leave NavItemType::getNavItem() unchanged (for NavItemModule and NavItemRedirect) and override it with a NavItemPage::getNavItem() ?

nadar commented 9 months ago

Yes, that sounds legit to me. And i would say we need a unit test for this change set :+1:

hbugdoll commented 9 months ago

Yes, that sounds legit to me.

Great.

And i would say we need a unit test for this change set 👍

I made an new unit test for this purpose and it fails with current getNavItem() – as desired: https://github.com/luyadev/luya-module-cms/actions/runs/8176492621/job/22356011023 Failed asserting that null is an instance of class "luya\cms\models\NavItem".

Changes on NavItemPage will come...

Edit: With https://github.com/luyadev/luya-module-cms/pull/415/commits/c981e198367af2a11bb5f5ab741a784092105143 the new unit test runs successfully 😃

nadar commented 8 months ago

:+1: thanks!