-
Notifications
You must be signed in to change notification settings - Fork 0
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 changes to support new solidus version #1
base: main
Are you sure you want to change the base?
Conversation
lib/solidus_subscriptions.rb
Outdated
@@ -2,7 +2,7 @@ | |||
|
|||
require 'solidus_core' | |||
require 'solidus_support' | |||
|
|||
require 'solidus_legacy_promotions' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, I am getting error uninitialized constant Spree::PromotionHandler
hence I have added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that class is never called in this extension. I suspect the problem is somewhere else. Do you have a full backtrace of the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kennyadsl I am not entirely sure about that https://github.com/search?q=repo%3Agms-electronics%2Fsolidus_subscriptions%20PromotionHandler&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I don't know what I was looking for. 😓 Also this extension adds some promotion rules for the legacy system. The quick way is to add legacy_promtionm as a dependency of this extension via its gemfile. But I guess the right way is to determine which promotion system has been used and call the right code based on that. @mamhoff if you have any guidance here, it would be appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @mamhoff!
@shahmayur001 you should be able to understand if there's the new or the legacy promotion system easily. Based on that we can avoid calling the code that only works with legacy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked through both uses of the Promotion Handler here. You can just remove them and everything should work as it should.
what happens in that case if a promotion is applied to a product?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kennyadsl @fthobe, I tested the dependency integration with my demo application and found the following:
-
I was able to resolve the promotion-related issue without adding the solidus_legacy_promotions gem to the subscription extension. Instead, I included the solidus_legacy_promotions gem in my demo application, which I created using Solidus 4.4.
-
@mamhoff , when you mention removing the Promotion Handler from the subscription extension, can I assume that promotions will then be activated directly through Solidus Core?
If the first point is the expected behavior, we can simply remove the dependency from the extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The extension should work with both promo systems. You can check for the existence of the old promotion system by checking
Object.const_defined?("Spree::Promotion")
. - Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are good than. @shahmayur001 great work @mamhoff thank you for your help!
@mamhoff Mayur is in the process of fixing the subscription gem, could you take a look if any changes are to be done to support your new promotion system? |
I don't use this gem; if you need it to be compatible with the new promotions system, you'll have to make sure it is yourself. |
@shahmayur001 how is the last commit related to the breadcrumbs issue? |
@fthobe This is used by the old classic frontend, which is no longer supported. @kennyadsl Hope it's okay to remove it. |
It's ok to remove the locale strings, not sure how that is related to breadcrumbs (as the commit message says). |
The strings of the core are overwritten when they are bundled with the subscriptions gem creating a mess with the new starter front-end looking for strings that have changed. The front-end therfor renders as visible in #304 Subscriptions break front-end breadcrumbs (screenshot below). |
5fc85a1
to
86cf5eb
Compare
@shahmayur001 Squash the commits to appease @kennyadsl , I have put the fixed items in the bottom of the description, use the standard template for a PR.
|
Draft PR
Dependency Updates, Form Adjustments, and New Privileges
This pull request resolves compatibility issues with Solidus version updates by adding necessary gem dependencies, modifying forms for proper field rendering, fixing jQuery issues with subscription creation, and updating permission sets. These changes align Solidus Subscriptions with the latest Solidus architecture.
Enhancements:
Dependency Addition
solidus_legacy_promotions
gem, which is required as promotions have been separated fromsolidus_core
.Form Rendering Restriction
jQuery Script Fix
subscribable_id
is stored.Permission Set Updates
Fixes: solidusio#304 solidusio#303 solidusio#302