Post by hcamelion » Sun Apr 21, 2019 10:31 am

Hi All,

Sorry, I have not posted in a while but I think I have a major bug that I just found.

Does anyone know if this was reported before and if so can provide the fix? If not I am going to fix it myself quickly. It has to do with having 15+ items in the cart and checking out and registering. The register/save ajax call logs in the customer and then deletes the cart and re-adds it. The problem is the subsequent ajax call (like country, shipping form, etc) do the same thing and the cart gets duplicated. Can anyone let me know if they can replicate it?

Here is my issue report:
https://github.com/opencart/opencart/issues/7375

If I fix this how do you suggest we fix it? I already fixed it by creating a define('JUST_LOGGED_IN') during the cart customer login method and look for that define in the cart constructor where the cart is refreshed but a longer term fix would be to remove the cart refresh from the constructor and adding a method for it in the cart class and then calling it from the cart customer login method. I searched the code for:

Code: Select all

$this->session->data['customer_id'] = 
It only exists in system/library/cart/customer.php:

Code: Select all

public function login($email, $password, $override = false) {
so we should add a method refreshCart() to system/library/cart/cart.php and call that method from login() and just put the current cart.php constructor code in it. I may do this and do a merge request in github for it if more experienced members think its the best fix.

Thanks!

Henry Weismann
877.44.MY.WEB (877.446.9932)
We can help with your Opencart Site - Opencart Web Developer

Image


User avatar
New member

Posts

Joined
Mon Jul 27, 2009 3:14 am
Location - Albany, NY, USA

Post by letxobnav » Sun Apr 21, 2019 11:20 am

I had that some time ago, same scenario but I did not investigate if it was for the same reason, pretty sure it is not because of the subsequent ajax calls though.

I changed this statement in system/library/cart/cart.php

Code: Select all

				// The advantage of using $this->add is that it will check if the products already exist and increaser the quantity if necessary.
				$this->add($cart['product_id'], $cart['quantity'], json_decode($cart['option']), $cart['recurring_id']);


to

Code: Select all

				// The advantage of using $this->add is that it will check if the products already exist and increaser the quantity if necessary.
				if (!$this->product_in_cart($cart['product_id'])) $this->add($cart['product_id'], $cart['quantity'], json_decode($cart['option']), $cart['recurring_id']);


so regardless of the stated "advantage", I check first if the product already exists in the cart and skip the "inceaser" part.

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by hcamelion » Sun Apr 21, 2019 2:36 pm

I think this code is meant to remove from cart and add the same things back right after deletion if the current session id has items in the cart with customer_id=0 and now has a customer id. When it adds them back it sets the right customer id and then the previous logged in cart is merged witht he logged out the cart. I was pretty sure it is because of the ajax calls. This is because I did a backtrace log entry between the inserts and updates and it indicated that startup had been called again before going into the same loop to delete and re-add. Also, I checked my access logs and I saw the ajax requests at the same timestamp as the second SQL statement that did the damage. The code that refreshed the cart also seems to happen on the cart constructor if the current session id has cart items belonging to cusotmer_id=0 which WOULD happen on a second ajax call if they are close together. Also, I just applied a fix so that this refresh cart code is only invoked on login and it all works now as intended. That's my analysis. I think your fix is just another way to fix it but from my understanding of the code your fix would probably at best not combine the previous logged in cart and the logged out cart and at worst the cart would be lost after login once the customer id is set in oc_cart. I would bet the cart items in your store will never get the customer_id column filled correctly in oc_cart after checkout registration step. I don't know what that means as far as the order being right in the end though.

Henry Weismann
877.44.MY.WEB (877.446.9932)
We can help with your Opencart Site - Opencart Web Developer

Image


User avatar
New member

Posts

Joined
Mon Jul 27, 2009 3:14 am
Location - Albany, NY, USA

Post by letxobnav » Mon Apr 22, 2019 12:29 pm

you may have a point, better to get that code out of the construct and call it as a function when needed.
Apart from the duplicates, it now also executes these queries needlessly every page load when a customer has signed in, one time is enough.

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by grazcata » Wed Jul 24, 2019 5:01 am

hcamelion wrote:
Sun Apr 21, 2019 2:36 pm
I think this code is meant to remove from cart and add the same things back right after deletion if the current session id has items in the cart with customer_id=0 and now has a customer id. When it adds them back it sets the right customer id and then the previous logged in cart is merged witht he logged out the cart. I was pretty sure it is because of the ajax calls. This is because I did a backtrace log entry between the inserts and updates and it indicated that startup had been called again before going into the same loop to delete and re-add. Also, I checked my access logs and I saw the ajax requests at the same timestamp as the second SQL statement that did the damage. The code that refreshed the cart also seems to happen on the cart constructor if the current session id has cart items belonging to cusotmer_id=0 which WOULD happen on a second ajax call if they are close together. Also, I just applied a fix so that this refresh cart code is only invoked on login and it all works now as intended. That's my analysis. I think your fix is just another way to fix it but from my understanding of the code your fix would probably at best not combine the previous logged in cart and the logged out cart and at worst the cart would be lost after login once the customer id is set in oc_cart. I would bet the cart items in your store will never get the customer_id column filled correctly in oc_cart after checkout registration step. I don't know what that means as far as the order being right in the end though.
Can you please provide the issue fix ?

Thank you.

Newbie

Posts

Joined
Wed Jul 24, 2019 4:59 am

Post by letxobnav » Wed Jul 24, 2019 10:42 am

I got rid of the statement in the cart class construct:

Code: Select all

if ($this->customer->getId()) {
...
}

Created new functions in the cart class:

Code: Select all

	public function renew_session_id_old_items ($customer_id) {
		$sql = "UPDATE " . DB_PREFIX . "cart SET session_id = '" . $this->db->escape($this->session->getId()) . "' WHERE api_id = '0' AND customer_id = '" . (int)$customer_id . "'";
		$this->db->query($sql);
	}
	
	
	public function transfer_items_to_new_customer ($customer_id) {
		$sql = "SELECT * FROM " . DB_PREFIX . "cart WHERE api_id = '0' AND customer_id = '0' AND session_id = '" . $this->db->escape($this->session->getId()) . "'";
		$cart_save_query = $this->db->query($sql);
		$sql = "DELETE FROM " . DB_PREFIX . "cart WHERE api_id = '0' AND customer_id = '0' AND session_id = '" . $this->db->escape($this->session->getId()) . "'";
		$this->db->query($sql);
		foreach ($cart_save_query->rows as $cart) {
			$sql = "DELETE FROM " . DB_PREFIX . "cart WHERE cart_id = '" . (int)$cart['cart_id'] . "'";
			$this->db->query($sql);
			$this->add($cart['product_id'], $cart['quantity'], json_decode($cart['option']), $cart['recurring_id']);
		}
	}


and I call those functions when a customer signs in.
that way it is only executed when needed and not on every page load for a signed in customer.

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by grazcata » Wed Jul 24, 2019 4:51 pm

Hi letxobnav , thank you for the fast reply.

Can you tell me please where to call the functions renew_session_id_old_items and transfer_items_to_new_customer ?

from catalog/controller/checkout/register.php ?

Newbie

Posts

Joined
Wed Jul 24, 2019 4:59 am

Post by letxobnav » Wed Jul 24, 2019 6:16 pm

I added it to:

catalog/controller/checkout/login.php

just after:

Code: Select all

		if (!$json) {
			// Unset guest
			unset($this->session->data['guest']);

			// change[662] giving old cart items of the customer the current session id
			$this->cart->renew_session_id_old_items ($this->customer->getId());
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());


catalog/controller/checkout/register.php

just after:

Code: Select all

			unset($this->session->data['guest']);
			
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());


catalog/controller/account/register.php

just after:

Code: Select all

			unset($this->session->data['guest']);
			
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());


catalog/controller/account/login.php

just after:

Code: Select all

if (($this->request->server['REQUEST_METHOD'] == 'POST') && $this->validate()) {
			// Unset guest
			unset($this->session->data['guest']);
			
			// change[662] giving old cart items of the customer the current session id
			$this->cart->renew_session_id_old_items ($this->customer->getId());
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by grazcata » Wed Jul 24, 2019 7:02 pm

Thank you letxobnav, that fixed the issue :)

Newbie

Posts

Joined
Wed Jul 24, 2019 4:59 am

Post by letxobnav » Wed Jul 24, 2019 8:03 pm

this one is better, previous one had to many deletes ( delete the saved entries all at once and also in the loop).

Code: Select all

public function transfer_items_to_new_customer ($customer_id) {
		// Once the customer is logged in we want to update the customers cart
		$sql = "SELECT * FROM " . DB_PREFIX . "cart WHERE api_id = '0' AND customer_id = '0' AND session_id = '" . $this->db->escape($this->session->getId()) . "'";
		$cart_save_query = $this->db->query($sql);
		foreach ($cart_save_query->rows as $cart) {
			$sql = "DELETE FROM " . DB_PREFIX . "cart WHERE cart_id = '" . (int)$cart['cart_id'] . "'";
			$this->db->query($sql);
			$this->add($cart['product_id'], $cart['quantity'], json_decode($cart['option']), $cart['recurring_id']);
		}
	}
I use the separate delete query in the loop so I can better trace it (is what is deleted also added) with an error_log statement if needed.

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by luke213 » Wed Jun 03, 2020 11:27 pm

letxobnav wrote:
Wed Jul 24, 2019 6:16 pm
I added it to:

catalog/controller/checkout/login.php

just after:

Code: Select all

		if (!$json) {
			// Unset guest
			unset($this->session->data['guest']);

			// change[662] giving old cart items of the customer the current session id
			$this->cart->renew_session_id_old_items ($this->customer->getId());
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());


catalog/controller/checkout/register.php

just after:

Code: Select all

			unset($this->session->data['guest']);
			
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());


catalog/controller/account/register.php

just after:

Code: Select all

			unset($this->session->data['guest']);
			
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());


catalog/controller/account/login.php

just after:

Code: Select all

if (($this->request->server['REQUEST_METHOD'] == 'POST') && $this->validate()) {
			// Unset guest
			unset($this->session->data['guest']);
			
			// change[662] giving old cart items of the customer the current session id
			$this->cart->renew_session_id_old_items ($this->customer->getId());
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());
Quick question is it possible to get these as updated files? I'm not quite grasping how these edits are setup. Or at least clarification before I go and edit;)

Background, having duplicate products during checkout, both on paypal as well as square. They are isolated to people using iphones and registering an account. Best I can tell those two variables if met cause the issues, but I do believe it's the same situation as above and I'd love to fix it.

Do I understand that after this segment of code:
if (!$json) {
// Unset guest
unset($this->session->data['guest']);
I would then insert this segment:
// change[662] giving old cart items of the customer the current session id
$this->cart->renew_session_id_old_items ($this->customer->getId());
// change[662] assigning the current guest cart items to the customer
$this->cart->transfer_items_to_new_customer ($this->customer->getId());
I appreciate the help;)

Luke

User avatar
New member

Posts

Joined
Thu Jul 08, 2010 2:35 am


Post by letxobnav » Thu Jun 04, 2020 8:25 am

Correct, 5 files need changes:

1) get rid of the cart merging logic in the construct function in system/library/cart/cart.php

remove this part:

Code: Select all

		if ($this->customer->getId()) {
			// We want to change the session ID on all the old items in the customers cart
			$this->db->query("UPDATE " . DB_PREFIX . "cart SET session_id = '" . $this->db->escape($this->session->getId()) . "' WHERE api_id = '0' AND customer_id = '" . (int)$this->customer->getId() . "'");

			// Once the customer is logged in we want to update the customers cart
			$cart_query = $this->db->query("SELECT * FROM " . DB_PREFIX . "cart WHERE api_id = '0' AND customer_id = '0' AND session_id = '" . $this->db->escape($this->session->getId()) . "'");

			foreach ($cart_query->rows as $cart) {
				$this->db->query("DELETE FROM " . DB_PREFIX . "cart WHERE cart_id = '" . (int)$cart['cart_id'] . "'");

				// The advantage of using $this->add is that it will check if the products already exist and increaser the quantity if necessary.
				$this->add($cart['product_id'], $cart['quantity'], json_decode($cart['option']), $cart['recurring_id']);
			}
		}

2) add these functions which do the same but will be called only when needed:

Code: Select all

	public function renew_session_id_old_items ($customer_id) {
		// We want to change the session ID on all the old items in the customers cart
		$sql = "UPDATE " . DB_PREFIX . "cart SET session_id = '" . $this->db->escape($this->session->getId()) . "' WHERE api_id = '0' AND customer_id = '" . (int)$customer_id . "'";
		$this->db->query($sql);
	}
	
	public function transfer_items_to_new_customer ($customer_id) {
		// Once the customer is logged in we want to merge the current guest items with the customer cart and empty the guest cart
		$sql = "SELECT * FROM " . DB_PREFIX . "cart WHERE api_id = '0' AND customer_id = '0' AND session_id = '" . $this->db->escape($this->session->getId()) . "'";
		$cart_save_query = $this->db->query($sql);
		foreach ($cart_save_query->rows as $cart) {
			$this->add($cart['product_id'], $cart['quantity'], json_decode($cart['option']), $cart['recurring_id']);
			$sql = "DELETE FROM " . DB_PREFIX . "cart WHERE cart_id = '" . (int)$cart['cart_id'] . "'";
			$this->db->query($sql);
		}
	}

3) call these functions in register and login controllers
2 function calls for login, giving the current session id to possible old customer items and merging the current guest items
1 function call for register as there cannot be any old customer items so just merging the current guest items

in catalog/controller/checkout/login.php and catalog/controller/account/login.php
just after:

Code: Select all

unset($this->session->data['guest']);
add:

Code: Select all

			// change[662] giving old cart items of the customer the current session id
			$this->cart->renew_session_id_old_items ($this->customer->getId());
			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());

in catalog/controller/checkout/register.php and catalog/controller/account/register.php
just after:

Code: Select all

unset($this->session->data['guest']);
add:

Code: Select all

			// change[662] assigning the current guest cart items to the customer
			$this->cart->transfer_items_to_new_customer ($this->customer->getId());

That is all.

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by luke213 » Thu Jun 04, 2020 8:41 am

Thanks so much for the detailed reply I'll give it a try and then see if it solves my issue;)

I really appreciate it and apologize for my thick head on the first go round;)

Luke

User avatar
New member

Posts

Joined
Thu Jul 08, 2010 2:35 am


Post by letxobnav » Thu Jun 04, 2020 8:57 am

That's ok, the original topic was becoming a little murky.

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by Retrofox » Fri Jul 24, 2020 4:04 pm

Hi mate, tried your solution a few times.
Issue is that when I add the modded "system/library/cart/cart.php" file it breaks the entire website (admin too) with a http 500 error.

Any ideas?
(Two orders in a row had this problem, order size not an issue)

(Opencart 3.0.2.0)

Newbie

Posts

Joined
Fri Oct 04, 2019 4:41 am

Post by letxobnav » Fri Jul 24, 2020 5:18 pm

then check your error logs.

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by imcdonald83 » Mon Aug 03, 2020 11:29 pm

@letxobnav: I tried this solution as I have been having this issue for time. I updated based on your easy to follow instructions but it's broken my website. The website is blank and the admin panel gives me this error.

Parse error: syntax error, unexpected 'public' (T_PUBLIC) in /home/xxxxxx/public_html/system/library/cart/cart.php on line 17

Any thoughts?

Newbie

Posts

Joined
Fri Mar 02, 2012 5:17 am

Post by letxobnav » Tue Aug 04, 2020 7:55 am

step 2 -> you put the functions in the wrong place in the cart class.

put them inside the class, i.e. somewhere after:

Code: Select all

private $data = array();

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan

Post by imcdonald83 » Tue Aug 04, 2020 3:56 pm

That's what i had done.

Step 1 remove the following
Image

Step 2 replace with this
Image

Newbie

Posts

Joined
Fri Mar 02, 2012 5:17 am

Post by letxobnav » Tue Aug 04, 2020 5:06 pm

no, you have put both functions inside the __construct function.

instruction 2 does not say replace what you removed in instruction 1, it says:
2) add these functions which do the same but will be called only when needed:

Crystal Light Centrum Taiwan
Extensions: MailQueue | SUKHR | VBoces

“Data security is paramount at [...], and we are committed to protecting the privacy of anyone who is associated with our [...]. We’ve made a lot of improvements and will continue to make them.”
When you know your life savings are gone.


User avatar
Expert Member

Posts

Joined
Fri Aug 18, 2017 4:35 pm
Location - Taiwan
Who is online

Users browsing this forum: No registered users and 5 guests