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

Fix page tagging condition: should_attach_to_menu? #1725

Merged

Conversation

mickenorlen
Copy link
Contributor

What is this pull request for?

When tagging a page I got this error due to menu_id being an empty string, so i updated the condition.

"MENU_ID"
""
  Alchemy::Node Load (0.5ms)  SELECT "alchemy_nodes".* FROM "alchemy_nodes" WHERE "alchemy_nodes"."id" = $1 LIMIT $2  [["id", nil], ["LIMIT", 1]]
   (0.3ms)  ROLLBACK

ActiveRecord::RecordNotFound Couldn't find Alchemy::Node with 'id'= in /app/_docker/data/bundle/ruby/2.7.0/gems/activerecord-6.0.2.1/lib/active_record/core.rb:177:in `find'
/app/_docker/data/bundle/ruby/2.7.0/gems/alchemy_cms-4.4.1/app/models/alchemy/page.rb:599:in `attach_to_menu!'

@mickenorlen mickenorlen requested a review from tvdeyen February 21, 2020 15:27
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. Although a duplicate of #1720 I prefer this fix because of readability. But please add a test.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 23, 2020

@mickenorlen rebasing with latest master will fix the CI build.

@mickenorlen mickenorlen force-pushed the fix-page-tagging-menu-attach-cond branch from 431a512 to 2f2a2e6 Compare February 24, 2020 08:44
@mickenorlen mickenorlen force-pushed the fix-page-tagging-menu-attach-cond branch from 2f2a2e6 to b55b41b Compare February 24, 2020 08:45
@mickenorlen mickenorlen force-pushed the fix-page-tagging-menu-attach-cond branch from b55b41b to a13d617 Compare February 24, 2020 08:45
@mickenorlen
Copy link
Contributor Author

I guess I have to continuously commit --amend and force push to stay in single commit? Why don't you just squash merge the whole PR instead?

@mickenorlen mickenorlen requested a review from tvdeyen February 24, 2020 08:48
@tvdeyen
Copy link
Member

tvdeyen commented Feb 24, 2020

I guess I have to continuously commit --amend and force push to stay in single commit? Why don't you just squash merge the whole PR instead?

I depends on how many commits are inside a PR. With small fixes like this it is ok to squash merge, but there are PRs that have a set of useful (git history wise) commits that needs to be preserved. Also it puts burden on the maintainer. And this burden is already very huge.

For this PR is is fine, though

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 for the fix 👍

@tvdeyen tvdeyen merged commit ffe648d into AlchemyCMS:master Feb 24, 2020
@tvdeyen tvdeyen mentioned this pull request Feb 24, 2020
tvdeyen added a commit that referenced this pull request Feb 25, 2020
- Do not use deprecated methods [#1737](#1737) ([tvdeyen](https://github.com/tvdeyen))
- Order contents by their position in its element [#1733](#1733) ([tvdeyen](https://github.com/tvdeyen))
- Eager load relations in elements trash [#1732](#1732) ([tvdeyen](https://github.com/tvdeyen))
- Run CI builds with Sprockets 3.7.2 [#1731](#1731) ([tvdeyen](https://github.com/tvdeyen))
- Re-organize development dependencies [#1730](#1730) ([tvdeyen](https://github.com/tvdeyen))
- Update pr template [#1729](#1729) ([tvdeyen](https://github.com/tvdeyen))
- Generate views without _view in the filename [#1728](#1728) ([tvdeyen](https://github.com/tvdeyen))
- Fix CI Builds [#1727](#1727) ([tvdeyen](https://github.com/tvdeyen))
- Fix page tagging condition: should_attach_to_menu?  [#1725](#1725) ([mickenorlen](https://github.com/mickenorlen))
- Fix Alchemy.user_class_name constant conflict [#1724](#1724) ([mickenorlen](https://github.com/mickenorlen))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants