All done! Please review!
Done! Works great and as expected, please review!
Alright, on it!
grevil → changed the visibility of the branch 3167716-unpublished-variation-access to hidden.
@jsacksick, so what are your final thoughts on this? I think the current approach is pretty good!
Done! Everything is green now!
I just ran into a similar issue with the same QueryException inside a scheduler integration for a custom entity. Both the entity data table and the entity revision data table had the "publish_on" column as expected, but the entity query got corrupted on SchdulerManager.php line 261:
$query->latestRevision();
The reason for this was that our custom entity had the "revision_table" entity Attribute set, but the "revision_data_table" key was missing. This lead to this issue because scheduler could not resolve the "revision_data_table" in the SchedulerManager $query->latestRevision() call, leading to the "publish_on" error not found (which is quite confusing, as it should say "could not resolve revision data table" but whatever...).
Here is the issue if anyone is interested: 🐛 Fix missing mandatory "AdContent" entity revision keys Active .
@lrwebks I think the comments @jonathan1055 gave here are more than enough, but I can understand that working with the schema can be confusing:
Add extra two lines in config/schema/scheduler.schema.yml to use the saved alias for the new third party settings.
In the scheduler.schema this:
node.type.*.third_party.scheduler: &third_party_settings_alias
type: mapping
label: 'Scheduler entity type settings'
mapping:
[...]
Simply aliases the mapping to "&third_party_settings_alias", which is then used to define the entity type third party scheduler schema objects:
media.type.*.third_party.scheduler: *third_party_settings_alias
Since we don't have access to the alias in our custom module we simply need to copy paste the node.type schema object and use our entity type name instead of node (we don't need the alias there of course).
In scheduler.install add a hook_update function to load the new view.
We simply add the update hook in our myModule.install file (only needed if the module already existed before this implementation)
This was the case for Media and Commerce Products (note from lrwebks: Two of the modules that scheduler supports internally, which is not what we want to achieve with our module)
Unsure what you are trying to say here?
Update README.md to include the new entity in the list of implementations.
This should be self-explanatory.
I think the Plugin implementation page could definitely need some work, but it's awesome that there even is one to begin with! Thanks for that @jonathan1055!
A few notes on the plugin documentation:
The plugin definition properties should be documented
Based on the names alone, it wasn't easy to see what exactly each definition property does. And correct me if I am wrong but aren't publishAction / unpublishAction and schedulerEventClass mandatory for third party modules (not living inside the scheduler module)? That would be a very important note!
Add extra two lines in config/schema/scheduler.schema.yml [...]
Add a comment, that we need to copy the original "node.type" schema definitions for third party modules
Create a new view definition and save it as
Add a comment, that the existing views inside scheduler.module can be an excellent template!
In scheduler.install add a hook_update function to load the new view.
Add a comment, that this is only mandatory if the third party module already existed before.
Scheduler Rules Integration
This should be inside an extra header, as it is not mandatory
General
I think it would be good to avoid explicit "scheduler" file names in the documentation. This might lead to confusion (e.g. scheduler.install => myModule.install, etc.)
And once again, thanks for this wonderful module @jonathan1055! I appreciate the work you are doing and the great support you are giving! If you are interested in the scheduler integration @lrwebks and I implemented, for the Drupal "Advertisement" module, take a look at https://git.drupalcode.org/project/ad/-/tree/11.x/modules/ad_content_sch...! ☺️
Alright, 🐛 Fix missing mandatory "AdContent" entity revision keys Active is resolved and all is done! Please review!
Damn, the missing revision data table entity key just swallowed half of my day...
POSTPONED on 🐛 Fix missing mandatory "AdContent" entity revision keys Active
All done, please review!
Side note @lrwebks please ALWAYS name task links identical to the route they represent. This is the Drupal standard and just costed me a bunch of time:
ad_content.admin:
title: 'Advertisements'
route_name: entity.ad_content.collection
base_route: system.admin_content
@ad_content.links.task.yml
There are a few update hooks in place, so it might already be possible. Needs to be tested.
Groups are now in place through creating custom "placements". Please create a follow-up feature request for the channels implementation, thx!
Alright, this does the trick. Otherwise LGTM!
Quick review by @Anybody regarding the new changes, but tested the update hook and it works as expected!
There is no update hook.
Hm weird...
Through the UI, I had no problem removing the module. Closing this again.
Done. Tested and works as expected!
grevil → created an issue.
@anybody yea probably a browser cache issue, since "placement" did not exist before.
Alright, everything else works great and as expected! Please review new adjustments @anybody.
I can not reproduce #7. When logged in as admin, I can see the ad and "placement" is part of the response data:
Yea, works great. Now to the final issue, pointed out in #7
Ok, this should do the trick. I will test it tommorow.
Ok, I forgot that by default the revisions are being used for the entity to be displayed, so we actually need to include that in the update hook as well.
Note, that this clears the revision data for "placement" (previously named size), but I really don't want to waste any more time on this 😅
FYI, nice for our Snippet collection:
https://www.drupal.org/docs/drupal-apis/update-api/updating-entities-and... →
https://www.drupal.org/node/2554097 →
Ok, fixed the update hook and tested, works as expected now! Continuing my review.
@lrwebks you forgot to modify the database scheme in the update hook, leading to the following error:
Drupal\Core\Entity\Query\QueryException: 'placement' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable() (line 373 of core/lib/Drupal/Core/Entity/Query/Sql/Tables.php).
LGTM! I wonder which idiot just recklessly removed the original composer.json. 😉😅
Ignore #49. Updated the former related issue, just a heads-up.
Still very eager to get this merged! 🤩
The Paypal integration was already implemented through 📌 Add PayPal support to Payment Element Gateway Active this issue only targets the Link implementation now.
grevil → made their first commit to this issue’s fork.
We have new information on this! See the comment in the parent issue here ✨ Print details from error messages returned by paypal Active .
Yes, we just tested the patch yesterday.
We get the "PAYMENT ACTION REQUIRED" as expected and following the attached link will bring us to PayPal, where we are seeing the correctly updated price. Unfortunately, as @anybody already stated, confirming the purchase will bring us into an endless PayPal loop.
https://developer.paypal.com/docs/checkout/standard/customize/overcharge... suggests, we should simply do a redirect to the provided link, but we somehow also need to provide a "return_url" (https://developer.paypal.com/docs/checkout/standard/customize/overcharge...). Otherwise, we won't get redirected to the shop after confirming the purchase.
But we are still unsure how to provide this "return_url". Using it as a query parameter on the redirect link didn't work properly, so we might need to make another sdk call. It might also be enough to simply provide a "return_url" and "cancel_url" on the "Create Order API" call (if that isn't already done).
Unfortunately, it isn't well documented on how to do this... @jsacksick any ideas?
All done! Please review!
Alright, that's it! Please review!
It is not worth changing the plugin manager "createInstance" method to additionally use a group to create a plugin instance.
Instead, simply prefixing the id will do the trick.
Current MR as patch.
Merged!
Done! Please review!
grevil → created an issue.
All done.
Now that we switched to the plugin manager, we might already have "proper" caching for the most part.
No, I meant a Oembed Widget Example. The widget url IS the endpoint.
Basically just a widget provider, that returns a JSON in the oembed format. No submodule.
All green! Merging.
Hey @tomtech, just wanted to ask how soon we can expect to get this reviewed / merged?
Also, I couldn't find an issue regarding the missing PayPal / Link PaymentMethodTypes, so I created one here: 🐛 Implement PayPal and Link "PaymentMethodType"s Active . Not sure if you guys plan to implement that yourself? Otherwise, we could also chime in to help implement the PayPal integration, as the "commerce_paypal" Express Checkout Buttons are currently no option for us (see 🐛 Anonymous user can't finish checkout using PayPal Express Checkout, when shipping rates exist Active ).
Thanks for the feedback! 🙂👍
grevil → created an issue.
Ok we could safely reproduce #10 now.
It definitely happens because of the missing shipping rate price in the total. Note, that this is NOT reproducible using sandbox mode.
Interestingly enough, the commerce stripe express checkout handles this correctly (see ✨ Stripe Express Checkout Element Integration Active ). But this might be a stripe thing. Stripe (through AmazonPay for example) communicates back and forth with the Server, so once AmazonPay has the address, they pass the address back to the server and the server can calculate the shipping rates based off the address given and send the available rates to stripe / amazon pay.
Please finish this @LRWebks
grevil → created an issue.
LGTM! Great work! 🎉🌈
grevil → created an issue.
Ok, back to needs work. Newest developments in 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active , show, that we need to properly handle the details and especially the links in a loop instead of resetting to the first issue / link available.
Ok, I gathered a few more information on this problem and I think I found the problem.
The problem is, that our shop overcharges the user above the amount he agreed to pay for, which is not allowed and against the "Overcapture requirements (PSD2)", on which PayPal implemented a way to reconfirm the new updated price through the "payer-action" link returned from the "PAYER_ACTION_REQUIRED" error response.
For more information, please take a look at:
https://www.paypal.com/ae/cshelp/article/overcapture-requirements-psd2-h...
https://developer.paypal.com/docs/multiparty/checkout/standard/customize...
This is easily reproducible using the express checkout buttons:
- An anonymous user buys a product through the express checkout cart button (we have no information on their address yet)
- He approves to pay the product costs via PayPal
- The user gets redirected to the shop. The shop now knows the address of the user (through by PayPal)
- The shop can now calculate the shipping rate based on the address given and patch the PayPal auction.
- When trying to finalize the purchase (capture the payment), PayPal returns a "PAYER_ACTION_REQUIRED" response containing a "payer-action" link, because the user approved the costs of the product only, not the costs of the product + shipping cost.
- The "payer-action" link redirects the user to PayPal, where he needs to reconfirm the new updated amount.
Unfortunately, there seems to be no error in sandbox mode for whatever reason. It seems like overcharging in sandbox mode is just fine, maybe there is a setting to tweak this behavior.
The remaining question is, whether this is something, that should be handled by the service provided through ✨ Print details from error messages returned by paypal Active . Or something else entirely. @tomtech @jsacksick what do you think about this?
Now, that we enabled the express checkout buttons, there are a LOT of messages like these in our shop. We temporarily disabled the express buttons, until this is properly handled.
When I apply the patch, update the db and add a new link, I get the following error:
TypeError: _menu_link_attributes_add_attributes(): Argument #2 ($options) must be of type array, null given, called in /var/www/html/web/modules/custom/menu_link_attributes/menu_link_attributes.module on line 36 in _menu_link_attributes_add_attributes() (line 47 of modules/custom/menu_link_attributes/menu_link_attributes.module).
@grevil looks like this needs an update hook to set this TRUE by default
Nope, it is good to go! "ViewsExposedFilterBlocksBlock" is a plugin, so the "defaultConfiguration()" method will do the job for us! LGTM!
Drupal 11 is stable for a while now, there won't be any more new patches from the bot! Setting this "Fixed".
clear() is public now! This is ready for review!
Great stuff! Thank you, @tomtech! 🎉
Something along these lines should suffice. I am unsure, whether "commerce_order.order.paid" is the correct event to use. But I couldn't find a better.
grevil → created an issue.
Magically fixed! 🌈
Sorry, we thought it might relate to 🐛 Orders are not completed Active .
@jsacksick, what do you think about the fix? It's pretty unclear, to us, how this fixes the issue.
Sadly, @alex.bukach did not reply.
We should indeed try to finish this (cleanly) as this could impact conversions and shop owners might not notice this issue at all.
Hmmm... Actually, I think we did it this way so that the alteration is done runtime rather than affecting the adjustment that is stored in DB. This way the adjustment label would always be on the current site language...
Can't remember exactly anymore. But we could still add a new setting (off by default) which prevents the subscriber to do anything, until activated!
EDIT: Or maybe there is something like an conditional subscriber in drupal / commerce? Unsure about that.
Alright! Seems to work as expected! We just had 2 test purchases inside a live environment using amazonPay and googlePay. Both worked as expected (Klarna was tested before inside a sandbox environment, which also worked great).
Setting this to "Needs review" for the Centarro people to take a final look at the changes!
New patch. Old patch had a bug I introduced, sorry for that.
@tomtech thanks for the feedback! And great to hear, that you guys are on it! Really appreciate the work of your team (and especially by @vmarchuk for working on this issue!).
I disabled support for link and PayPal. We will test it live now and come back to you guys again!
Static patch of the current MR attached.
Sorry, back to "Needs work" regarding PayPal.
Alright, this is pretty far now.
Regarding the payment methods:
Klarna
Works great and as expected.
AmazonPay
Works great and as expected.
GooglePay
Works great and as expected.
ApplePay
Not tested. I do not have Safari.
Link
Could not test link, since I couldn't log into my account for some reason
PayPal
After confirming the payment I get an error and get sent to the "checkout/{order_id}/order_information" page with the error:
Payment failed at the payment server. Please review your information and try again.
There are still a couple of console errors and I feel like a few parts of code should use existing commerce services instead of "reinventing the wheel" (e.g. the shipping address validation). But generally speaking, this is fairly close!
We will test it in a live environment tommorow and give further feedback.
Ignore #40 ✨ Stripe Express Checkout Element Integration Active we now skip the adjustment if it is of type "tax" and already included in the line item price. Meaning, the original implementation of "commerce_stripe_get_express_checkout_order_line_items" by @vmarchuk is restored again (with the mentioned check included).
But I am still unsure if is the correct approach, to pass the tax as a line item. But according to the Stripe AI assistan it is:
Certainly! Here's how you can add tax to a line item using Stripe.js without automatic tax calculation:
example.js
const stripe = Stripe('your_publishable_key');
const lineItems = [
{
price_data: {
currency: 'usd',
unit_amount: 2000,
product_data: {
name: 'T-shirt',
},
},
quantity: 1,
},
{
price_data: {
currency: 'usd',
unit_amount: 200,
product_data: {
name: 'Sales Tax',
},
},
quantity: 1,
}
];
stripe.checkout.sessions.create({
mode: 'payment',
line_items: lineItems,
success_url: 'https://example.com/success',
cancel_url: 'https://example.com/cancel',
}).then(function(session) {
// Handle the session
});
In this example:
We define two line items in the lineItems array:
The T-shirt priced at $20.00 (2000 cents)
The sales tax as a separate line item, priced at $2.00 (200 cents)
We don't use the automatic_tax parameter, as we're manually adding the tax as a separate line item.
But on docs provided as resources by the AI I couldn't find this approach. But yea seems good enough.
as mentioned, waiting for other community members to chime in here, especially the ones that contributed patches previously. But from a pure code point of view, this looks good now
I don't think anyone will chime in here anymore. Can we get this fixed?
Ok, I found the issue. Adjustments were added as "extra_line_items", leading to this error.
Although this code seems intentional, I could not yet find the reason it was coded this way. @vmarchuk could you give feedback, on why the adjustments were added as line items? It might be needed for "onShippingRateChange()" and "onShippingAddressChange()" but not passed to the js eventually? Or we need to revert https://git.drupalcode.org/project/commerce_stripe/-/merge_requests/129/... again and just add a check?
Awesome! Thanks, @dydave and @ressa for your quick back 2 back finalization of this! 😍