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

Menu item should not match url if match_path is set #5643

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

sascha-karnatz
Copy link

@sascha-karnatz sascha-karnatz commented Feb 7, 2024

Summary

The menu item should not match the url if a match_path prevents this behavior. Previously the match_path method went truthy, if the url of the menu item started with the same url as the request regardless of the given match_path - configuration.

Example

The regex in the match_path of menu item 1 should highlight only if the url ends with "bar":

Menu Item 1: {url: '/foo/bar', match_path: %r{/bar$/} } 
Menu Item 2: {url: '/foo/bar_baz'}
Request: /foo/bar_baz

In the prevous implementation both menu items got highlighted, because they started with the same url. In the implemation of this Pull Request only the second menu item will be highlighted.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • ✅ I have added automated tests to cover my changes.

@sascha-karnatz sascha-karnatz requested a review from a team as a code owner February 7, 2024 11:02
@github-actions github-actions bot added the changelog:solidus_backend Changes to the solidus_backend gem label Feb 7, 2024
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks. Makes sense to me. The test failures are unrelated. Fix is in #5644

The menu item should not match the url if a match_path prevents this behavior. Previously the match_path method went truthy, if the url of the menu item started with the same url as the request regardless of the given match_path - configuration.
@sascha-karnatz sascha-karnatz force-pushed the fix-menu-item-menu-path branch from 1a74a2d to 26fa1c1 Compare February 7, 2024 15:52
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bb80c3a) 88.55% compared to head (26fa1c1) 88.55%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5643   +/-   ##
=======================================
  Coverage   88.55%   88.55%           
=======================================
  Files         685      685           
  Lines       16407    16408    +1     
=======================================
+ Hits        14529    14530    +1     
  Misses       1878     1878           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tvdeyen tvdeyen added backport-v4.2 Backport this pull-request to v4.2 backport-v4.3 Backport this pull-request to v4.3 backport-v4.0 Backport this pull-request to v4.0 backport-v4.1 Backport this pull-request to v4.1 labels Feb 7, 2024
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

👍

@tvdeyen tvdeyen removed backport-v4.0 Backport this pull-request to v4.0 backport-v4.1 Backport this pull-request to v4.1 labels Feb 8, 2024
@tvdeyen tvdeyen merged commit 248a6e6 into solidusio:main Feb 8, 2024
24 checks passed
@kennyadsl
Copy link
Member

@tvdeyen I'm not sure about all those backport labels. The changed code was introduced with #5309, merged on v4.2

Copy link

github-actions bot commented Feb 8, 2024

💚 All backports created successfully

Status Branch Result
v4.2
v4.3

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@kennyadsl
Copy link
Member

EDIT: you are already on that. ❤️

@tvdeyen
Copy link
Member

tvdeyen commented Feb 8, 2024

EDIT: you are already on that. ❤️

Yes, thankfully @sascha-karnatz pointed me to it as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v4.2 Backport this pull-request to v4.2 backport-v4.3 Backport this pull-request to v4.3 changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants