? ? Pending

User tests: Successful: Unsuccessful:

avatar rajkumaar23
rajkumaar23
11 Jan 2020

Summary of Changes

Removes redundant calculation of limit value in pagination.

Testing Instructions

  1. Code review
  2. Ensure pagination doesn't break after this PR

Expected result

Actual result

Documentation Changes Required

Nothing

avatar rajkumaar23 rajkumaar23 - open - 11 Jan 2020
avatar rajkumaar23 rajkumaar23 - change - 11 Jan 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 11 Jan 2020
Category Layout
avatar rajkumaar23 rajkumaar23 - change - 11 Jan 2020
Labels Added: ?
avatar brianteeman
brianteeman - comment - 11 Jan 2020

What makes you feel it's not needed

avatar rajkumaar23
rajkumaar23 - comment - 11 Jan 2020

What makes you feel it's not needed

If you look at the previous line, the content is being stored in a variable $limit.

	if ($item->base > 0)
	{
		$limit = 'limitstart.value=' . $item->base;
	}
	else
	{
		$limit = 'limitstart.value=0';
	}

Hence, it is meaningless to recalculate.

avatar richard67
richard67 - comment - 11 Jan 2020

@brianteeman It seems the same calculation is already done here:

if ($item->base > 0)
{
$limit = 'limitstart.value=' . $item->base;
}
else
{
$limit = 'limitstart.value=0';
}
.

So the PR could be right.

@rajkumaar23 Could you provide some testing instructions?

avatar rajkumaar23
rajkumaar23 - comment - 11 Jan 2020

@brianteeman It seems the same calculation is already done here:

if ($item->base > 0)
{
$limit = 'limitstart.value=' . $item->base;
}
else
{
$limit = 'limitstart.value=0';
}

.

So the PR could be right.

@rajkumaar23 Could you provide some testing instructions?

Sorry, I'm new to Joomla. I have no idea about how tests work here. Could you please help ?

avatar richard67
richard67 - comment - 11 Jan 2020

@rajkumaar23 Just add some testing instructions for human testers to the description in your PR where the head lines from the PR template say so:

Testing Instructions

Expected result

Actual result

For example you could simply write "Code review" below "Testing inctructions".

If you want to be sure that you made no mistake which could be missed by some reviewer you also could write instructions for a real test by a human, e.g.:

  1. Apply the patch of this PR.
  2. Make sure that pagination links in frontend still work.

(or so)

Here for this PR it is trivial, but our contribution rules require to fill out the necessary information in the description.

Thank you for your PR.

avatar rajkumaar23 rajkumaar23 - change - 11 Jan 2020
The description was changed
avatar rajkumaar23 rajkumaar23 - edited - 11 Jan 2020
avatar rajkumaar23
rajkumaar23 - comment - 11 Jan 2020

@richard67
Sure, thanks. I've updated my PR description ?

avatar richard67 richard67 - change - 11 Jan 2020
Title
Removes unnecessary limit calculation in pagination
[4.0] Removes unnecessary limit calculation in pagination
avatar joomla-cms-bot joomla-cms-bot - edited - 11 Jan 2020
avatar richard67
richard67 - comment - 11 Jan 2020

@rajkumaar23 When we make PRs for the 4.0-dev branch we're used to have "[4.0] " at the beginning of the title, so we can quickly see it is not for staging (= 3.9.x-dev) or 3.10-dev. Is not a must but good practice. I've just updated the title for you. Will test soon, but as I could already see by code review I am sure the result will be success.

avatar rajkumaar23
rajkumaar23 - comment - 11 Jan 2020

@richard67 Sure, I'll follow that format. And, thanks for the help :)

avatar richard67
richard67 - comment - 11 Jan 2020

@rajkumaar23 Another suggestion: I see you were using your 4.0-dev branch for this PR. This is ok as long as you do not make several PRs for the 4.0-dev branch of Joomla.

But as you maybe can imagine, new contributors are always welcome, and so if you have some free time and find other issues to fix you might make more than one PR at a time. We (Joomla community) would be happy about that.

For working on several issues it will be necessary to create a new branch for making a PR. Depending on the branch for which the PR will be, you will make this new branch based on the 4.0-dev branch or the staging branch of the CMS.

Maybe you know all that already and this time it was just faster not to do it. But if that is new to you, you can find some useful information here: https://docs.joomla.org/Using_the_Github_UI_to_Make_Pull_Requests and here https://docs.joomla.org/Git_branching_quickstart and at a few other places linked on https://docs.joomla.org/Portal:Joomla!_Code_Contributors.

avatar richard67 richard67 - test_item - 11 Jan 2020 - Tested successfully
avatar richard67
richard67 - comment - 11 Jan 2020

I have tested this item successfully on 1334f2b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27480.

avatar Quy Quy - test_item - 11 Jan 2020 - Tested successfully
avatar Quy
Quy - comment - 11 Jan 2020

I have tested this item successfully on 1334f2b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27480.

avatar Quy Quy - change - 11 Jan 2020
Status Pending Ready to Commit
avatar Quy
Quy - comment - 11 Jan 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27480.

avatar Quy Quy - change - 11 Jan 2020
Labels Added: ?
avatar wilsonge
wilsonge - comment - 11 Jan 2020

Thanks for your first PR to Joomla ?

avatar wilsonge wilsonge - change - 11 Jan 2020
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-01-11 17:11:49
Closed_By wilsonge
avatar wilsonge wilsonge - close - 11 Jan 2020
avatar wilsonge wilsonge - merge - 11 Jan 2020
avatar rajkumaar23
rajkumaar23 - comment - 11 Jan 2020

Thanks for your first PR to Joomla ?

Thank you! ?️ Looking forward to contribute more.

Add a Comment

Login with GitHub to post a comment