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

Fixed navItem relation for inactive page versions #415

Merged
merged 5 commits into from
Mar 8, 2024

Conversation

hbugdoll
Copy link
Member

@hbugdoll hbugdoll commented Mar 5, 2024

What are you changing/introducing

  • Fixed navItem relation for NavItemType NavItemPage
    • by overriding getNavItem() and therein changing relation from "reverse" directionhasOne(..., navItemFK => navItemTypePK) into hasOne(..., navItemPK => navItemTypeFK)

What is the reason for changing/introducing

  • navItem was missing for inactive page versions

QA

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues Closes #414

@nadar
Copy link
Contributor

nadar commented Mar 6, 2024

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

@hbugdoll
Copy link
Member Author

hbugdoll commented Mar 6, 2024

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

nadar commented Mar 6, 2024

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

@hbugdoll
Copy link
Member Author

hbugdoll commented Mar 6, 2024

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 c981e19 the new unit test runs successfully 😃

@nadar
Copy link
Contributor

nadar commented Mar 8, 2024

👍 thanks!

@nadar nadar merged commit 75a2afe into luyadev:master Mar 8, 2024
7 of 8 checks passed
@hbugdoll hbugdoll deleted the fix-navitem-relation branch March 8, 2024 16:26
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.

Env option pageObject is incomplete in inactive page versions
2 participants