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
Active 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
Active 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
Active 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
Active 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).

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
Active Member

Posts

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

Users browsing this forum: No registered users and 4 guests