Function if comparision outputting false value incorrectly.

[Adamhumbug]

Hi All,

I think this may be a bit of a slog to get to the answer here as there are a fair few function calls and quite long functions.

I have a function that is called by ajax to create some form items.  This works fine. The form can be filled out and all of the data goes to the DB.

Now, when wanting to recreate the form, filled with the data, so that users can edit i have part of the function that should be setting the selected item in an option list that is outputting the value where no match has been found rather than the value when a match has been found.

here is the big function

function getItemsForQuote($itemType, $currency, $qty = null, $startDate = null, $endDate = null, $itemId = null, $notes = null)
{
	require 'includes/dbconn.php';



	$sql = "SELECT id, name, price_uk, price_us, price_ca, price_au, price_eu, billing_freq from item where type = :item_type";
	$stmt = $pdo->prepare($sql);
	$stmt->bindParam(':item_type', $itemType);
	$stmt->execute();
	$items = $stmt->fetchAll(PDO::FETCH_ASSOC);
	if ($items) {
		$qtyArray = array(
			1 => '1',
			2 => '2',
			3 => '3',
			4 => '4',
			5 => '5',
			6 => '6',
			7 => '7',
			8 => '8',
			9 => '9',
			10 => '10',
			11 => '11',
			12 => '12',
			13 => '13',
			14 => '14',
			15 => '15',
			16 => '16',
			17 => '17',
			18 => '18',
			19 => '19',
			20 => '20'
		);

		$out = "<div class='row mb-3 completeLine'><div class='col-1'>
					<select name='itemQty[]' class='itemQty form-select'>";
		foreach ($qtyArray as $key => $value) {
			if ($qty == $key) {
				$selected = 'selected';
				$out .= "<option selected='{$selected}' value='{$key}'>{$value}</option>";
			}
			$out .= "<option value='{$key}'>{$value}</option>";
		}
		$out .= "</select>
				</div>
				<div class='col-4'>
						<select name='itemSelected[]' class='itemSelected form-select'><option selected disabled>Please Select</option>";
						
		foreach ($items as $item) {
			if ($itemId == $item['id']) {
				$selected = 'selected';
				$out .= "<option selected='{$selected}' value='{$item['id']}'>{$item['name']}</option>";
			}
			$out .= "<option data-billingfreq='{$item['billing_freq']}' data-priceuk='{$item['price_uk']}' data-priceus='{$item['price_us']}' data-priceca='{$item['price_ca']}' data-priceau='{$item['price_au']}' data-priceeu='{$item['price_eu']}'  value='{$item['id']}'>{$item['name']}</option>";
		}
		$out .= "</select>
				</div>
					<div class='col-2'>
							<input name='fromDate[]' type='date' class='form-control from-date'>
						</div>
						<div class='col-2'>
							<input name='toDate[]' type='date' class='form-control to-date'>
						</div>
						<div class='col-2'>
							<input name='lineTotal[]' type='text' disbaled placeholder='Total' class='line-total form-control'>
						</div>
						<div class='col-1'>
							<div class='btn btn-primary w-100'>Opt</div>
						</div>
						<div class='col-7 mt-3'>
							<div class='input-group mb-3'>
								<span class='input-group-text'>Notes</span>
								<input name='lineNotes[]' type='text' class='line-notes form-control' placeholder='Add notes for line item'>
							</div>
						</div>
						<div class='col-3 mt-3'>
							<div class='input-group mb-3'>
								<span class='input-group-text pricePerWeek'>PPW</span>
								<input type='text' class='pricePerWeek form-control' placeholder='PPW'>
							</div>
						</div>
						<div class='col-2 mt-3'>
							<div class='input-group mb-3'>
								<span class='input-group-text noOfWeeks'>Weeks</span>
								<input type='text' class='noOfWeeks form-control' placeholder='Weeks'>
							</div>
						</div>
					</div>
				</div>
				";
	}
	return $out;
}

The section that is not working correctly is

foreach ($items as $item) {
			if ($itemId == $item['id']) {
				$selected = 'selected';
				$out .= "<option selected='{$selected}' value='{$item['id']}'>{$item['name']}</option>";
			}
			$out .= "<option data-billingfreq='{$item['billing_freq']}' data-priceuk='{$item['price_uk']}' data-priceus='{$item['price_us']}' data-priceca='{$item['price_ca']}' data-priceau='{$item['price_au']}' data-priceeu='{$item['price_eu']}'  value='{$item['id']}'>{$item['name']}</option>";
		}

itemId is being passed into this function correctly i believe which has been checked with the function that calls this one below.

The $itemId which is passed in = 2 and the $item['id'] that is being pulled in from the database in this query should match.

 

The above function is called from the following function

function rebuildQuote($quoteId, $version)
{
	include 'includes/dbconn.php';
	$sql = "SELECT item_id as itemId,quote.currency as quoteCurrency, item.type as itemType, quantity, from_date, to_date, price_quoted from quote_items
			inner join item on item.id = quote_items.item_id
			inner join quote on quote.id = quote_items.quote_id
			where quote_id = :quoteId and quote_items.version = :version";
	$stmt = $pdo->prepare($sql);
	$bindData = [
		'quoteId' => $quoteId,
		'version' => $version
	];
	$stmt->execute($bindData);
	$items = $stmt->fetchAll(PDO::FETCH_ASSOC);
	$out = "";
	if ($items) {
		foreach ($items as $item) {
			$out .= getItemsForQuote($item['itemType'], $item['quoteCurrency'], $item['quantity'], $item['itemId']);
			//  $out .= $item['itemId']."and";
		}
	} else {
		$out .= "no items";
	}

	return $out;
}

You will see that i have been changing the $out at the end to check what variables are being sent into the function call.

I have been going around the houses on this but any pointers in the right direction would be appreicted.  I am hoping that it is a "cant see the woods for the trees" issue.

 

 

 

[Adamhumbug]

If more information is needed for this i will be happy to supply it, i didnt want to overload with too much info in one place.

[maxxd]

I'm pretty sure I don't understand the issue you're having, but right off the top I see this. This is your function definition:

function getItemsForQuote($itemType, $currency, $qty = null, $startDate = null, $endDate = null, $itemId = null, $notes = null)

and this is what you're calling:

getItemsForQuote($item['itemType'], $item['quoteCurrency'], $item['quantity'], $item['itemId']);

You're passing the itemId as the startDate parameter.

[mac_gyver]

this code is killing your database server with connections. a database connection is one of the slowest, resource consuming operations you can do. your main application code should make one database connection. you would then pass it as a call-time parameter to any function that needs it.

the getItemsForQuote function has the wrong responsibility. it is mixing the database specific code with the presentation code,  it should only get the data, like its name indicates and my not actually be needed to get the data one item at a time. see the next point.

this code is executing a select query within a loop (getting all the items for the item_type of the current item), which is also inefficient. even if there's only one item per item_type in a quote, and even worse if there is more than one, you should get all this data in one query, index it by the item_type when you fetch it, then access it when building each itemSelected select/option menu.

[Adamhumbug]

1 hour ago, maxxd said:

I'm pretty sure I don't understand the issue you're having, but right off the top I see this. This is your function definition:

function getItemsForQuote($itemType, $currency, $qty = null, $startDate = null, $endDate = null, $itemId = null, $notes = null)

and this is what you're calling:

getItemsForQuote($item['itemType'], $item['quoteCurrency'], $item['quantity'], $item['itemId']);

You're passing the itemId as the startDate parameter.

Thank you, this was the answer! 🥱

[Adamhumbug]

33 minutes ago, mac_gyver said:

this code is killing your database server with connections. a database connection is one of the slowest, resource consuming operations you can do. your main application code should make one database connection. you would then pass it as a call-time parameter to any function that needs it.

the getItemsForQuote function has the wrong responsibility. it is mixing the database specific code with the presentation code,  it should only get the data, like its name indicates and my not actually be needed to get the data one item at a time. see the next point.

this code is executing a select query within a loop (getting all the items for the item_type of the current item), which is also inefficient. even if there's only one item per item_type in a quote, and even worse if there is more than one, you should get all this data in one query, index it by the item_type when you fetch it, then access it when building each itemSelected select/option menu.

I think i need to read this a few more times and see if i can make that work.  I may be back for this comment.  Thanks for taking the time, it is appreciated.

[mac_gyver]

an additional point about the function implementation. the purpose of the markup being built inside of getItemsForQuote is to edit the existing item row values. to do so, you need to supply all those initial existing values to the code, which actually changes to become the submitted form data, when you redisplay the form upon a validation error. do NOT add a discrete call-time parameter for each of these values. simply pass an array of values as one call-time parameter. when you initially produce the form, the array of data would be that which you query for and fetch from the database. when you redisplay the form upon a validation error, the array of data would be the submitted form data.

[mac_gyver]

another issue with the posted code.

when you are building the 'selected' option choices, in both the quantity and the item menus, you are outputting the option with the selected attribute when the existing value is the same as the value being output AND you are repeating the same option choice without the selected attribute. e.g. if the selected quantity is 3, you are outputting an option choice for 3 that is selected, followed by another option choice for 3 that is not selected.

i doubt this is the intended result. the only conditional logic should be to determine what the selected attribute should be, the option markup should only be output once per value, that is either selected or not. note: you can simply put the 'selected' attribute in or leave it out. you don't have to put it in AND set it to a (true, i.e. any non-empty string) value to cause the option to be selected.

if by some chance this is the intended result, in the case of the item select/option menu, the data-... attribute(s) are used in the lineTotal/line-total calculation, so they must be output in every option choice. the current code is not outputting them in the 'selected' option choice, which will prevent the calculation code from producing the total.

[Adamhumbug]

3 hours ago, mac_gyver said:

another issue with the posted code.

when you are building the 'selected' option choices, in both the quantity and the item menus, you are outputting the option with the selected attribute when the existing value is the same as the value being output AND you are repeating the same option choice without the selected attribute. e.g. if the selected quantity is 3, you are outputting an option choice for 3 that is selected, followed by another option choice for 3 that is not selected.

i doubt this is the intended result. the only conditional logic should be to determine what the selected attribute should be, the option markup should only be output once per value, that is either selected or not. note: you can simply put the 'selected' attribute in or leave it out. you don't have to put it in AND set it to a (true, i.e. any non-empty string) value to cause the option to be selected.

if by some chance this is the intended result, in the case of the item select/option menu, the data-... attribute(s) are used in the lineTotal/line-total calculation, so they must be output in every option choice. the current code is not outputting them in the 'selected' option choice, which will prevent the calculation code from producing the total.

All good points here - you are correct and i have ammended my code.

[Adamhumbug]

18 hours ago, mac_gyver said:

an additional point about the function implementation. the purpose of the markup being built inside of getItemsForQuote is to edit the existing item row values. to do so, you need to supply all those initial existing values to the code, which actually changes to become the submitted form data, when you redisplay the form upon a validation error. do NOT add a discrete call-time parameter for each of these values. simply pass an array of values as one call-time parameter. when you initially produce the form, the array of data would be that which you query for and fetch from the database. when you redisplay the form upon a validation error, the array of data would be the submitted form data.

for this do you mean that

function getItemsForQuote($itemType, $currency, $qty = null, $itemId = null, $startDate = null, $endDate = null,$priceQuoted = null,  $notes = null)

should be replaced with one array containing all of this data rather than lots of individual elements?

[mac_gyver]

1 hour ago, Adamhumbug said:

should be replaced with one array containing all of this data rather than lots of individual elements?

yes. just name it $item, because that is what it is in the code that's calling that function. then reference elements in item,  e.g. $item['quantity'] inside the function code.

this actually would have prevented the initial error at the top of this thread, because there would be no need to match up all the  different parameters. it will also allow you to add more values, such as the lineNotes, simply by selecting that data where you are querying for it, then using that data inside the function. you won't have to edit the list of call-time parameters every time you change the data being passed.

[Adamhumbug]

37 minutes ago, mac_gyver said:

yes. just name it $item, because that is what it is in the code that's calling that function. then reference elements in item,  e.g. $item['quantity'] inside the function code.

this actually would have prevented the initial error at the top of this thread, because there would be no need to match up all the  different parameters. it will also allow you to add more values, such as the lineNotes, simply by selecting that data where you are querying for it, then using that data inside the function. you won't have to edit the list of call-time parameters every time you change the data being passed.

great advice, thank you for this.