Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prompt before reducing number of rows #13987

Merged
merged 10 commits into from Feb 14, 2018

Conversation

Projects
None yet
4 participants
@shucon
Copy link
Contributor

commented Feb 10, 2018

Fixes: #13952

Signed-off-by: Saksham Gupta shucon01@gmail.com

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests
Prompt before reducing number of rows
Fixes: #13952

Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@codecov

This comment has been minimized.

Copy link

commented Feb 10, 2018

Codecov Report

Merging #13987 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #13987      +/-   ##
============================================
+ Coverage     55.43%   55.44%   +<.01%     
- Complexity    14358    14360       +2     
============================================
  Files           494      494              
  Lines         70453    70459       +6     
============================================
+ Hits          39059    39063       +4     
- Misses        31394    31396       +2
@@ -69,6 +69,8 @@ function () {
= __('Do you really want to delete the search "%s"?');
$js_messages['strConfirmNavigation']
= __('You have unsaved changes; are you sure you want to leave this page?');
$js_messages['strConfirmRowChange']
= __('You have unsaved changes; are you sure you want to reduce number of rows?');

This comment has been minimized.

Copy link
@ibennetch

ibennetch Feb 10, 2018

Member

Your message is okay, but I've been trying to make error messages and warnings more user-friendly. How does "You are trying to reduce the number of rows, but have already entered data in those rows which will be lost. Do you wish to continue?" sound to everyone?

@ibennetch

This comment has been minimized.

Copy link
Member

commented Feb 10, 2018

I haven't tested it yet but this looks quite good.

Edit Error Message
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
js/ajax.js Outdated
var row = document.getElementById('insert_rows');
if (row !== null) {
var row_count = Number(row.value);
row.addEventListener("change", function(){

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Feb 10, 2018

Member

An event listener already exists for this selector. Why not use it?

$(document).on('change', '#insert_rows', function (event) {

shucon added some commits Feb 10, 2018

Changed code location
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Fix minor bug
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Improve code quality
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Codacy Fix
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
js/ajax.js Outdated
} else {
return newCount;
}
},
/**

This comment has been minimized.

Copy link
@devenbansod

devenbansod Feb 11, 2018

Member

@shucon I'm not sure if this file would be an ideal place to add this function. It's not related to AJAX as such, more related to a specific page.

This comment has been minimized.

Copy link
@shucon

shucon Feb 11, 2018

Author Contributor

@devenbansod I did this for two reasons.

  1. A similar function for the same page is written here (line 249).
  2. I had to use AJAX.lockedTargets . Although I could use it anywhere but writing it here seemed right because of the above reason.

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Feb 14, 2018

Member

I have to agree with Deven.

This comment has been minimized.

Copy link
@shucon

shucon Feb 14, 2018

Author Contributor

Should I transfer the code to tbl_change.js ?

shucon added some commits Feb 12, 2018

Minor Bug fix
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Travis Rerun
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
js/ajax.js Outdated
*/
rowCheck: function (row, rowCount) {
var newCount = Number(row.value);
if (newCount < rowCount && !jQuery.isEmptyObject(AJAX.lockedTargets)

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Feb 14, 2018

Member

You don't need to check if the number of rows the user entered is less than or greater than the number of rows on the page. This check already exists, you only need to add the confirmation message.

This comment has been minimized.

Copy link
@shucon

shucon Feb 14, 2018

Author Contributor

Where does this check exists? Also , I would have to provide some condition to show the confirmation message.

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Feb 14, 2018

Member

This check is already done when adding or removing lines from the page.

if (curr_rows < target_rows) {

} else if (curr_rows > target_rows) {

This comment has been minimized.

Copy link
@shucon

shucon Feb 14, 2018

Author Contributor

Thanks @mauriciofauth I've done the required changes.

* Displays warning if number of rows are reduced
* and chances of data loss.
*/
rowCount = AJAX.rowCheck(row, rowCount);

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Feb 14, 2018

Member

You are returning a value, but you are not using it for anything.

This comment has been minimized.

Copy link
@shucon

shucon Feb 14, 2018

Author Contributor

I'm using the returned value to update the old row count and compare it with the new count that the user wants to check the rowCheck condition.

Requested Changes
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
*/
if (jQuery.isEmptyObject(AJAX.lockedTargets) || (!jQuery.isEmptyObject(AJAX.lockedTargets)
&& confirm(PMA_messages.strConfirmRowChange) === true)
) {

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Feb 14, 2018

Member

Don't perform complex actions in if statements. Put this jQuery call into a variable.

This second jQuery call is redundant and can be removed. The right side of the || will only run if the left side is false.

This comment has been minimized.

Copy link
@shucon

shucon Feb 14, 2018

Author Contributor

@mauriciofauth this jQuery call is used in ajax.js too without variable

curr_rows--;
}
} else {
document.getElementById('insert_rows').value = curr_rows;

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Feb 14, 2018

Member

Fix the indentation of this block.

/**
* Displays warning if number of rows are reduced
* and chances of data loss.
*/

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Feb 14, 2018

Member

Is this comment really necessary?

This comment has been minimized.

Copy link
@shucon

shucon Feb 14, 2018

Author Contributor

I left it by mistake , thanks.

Fix code quality
Signed-off-by: Saksham Gupta <shucon01@gmail.com>

@mauriciofauth mauriciofauth merged commit ff06650 into phpmyadmin:master Feb 14, 2018

5 checks passed

DCO All commits have a DCO sign-off from the author
Scrutinizer Analysis: No new issues – Tests: passed
Details
codecov/patch 100% of diff hit (target 55.43%)
Details
codecov/project 55.44% (+<.01%) compared to d61fba6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mauriciofauth mauriciofauth self-assigned this Feb 14, 2018

@mauriciofauth mauriciofauth added this to the 4.8.0 milestone Feb 14, 2018

@mauriciofauth

This comment has been minimized.

Copy link
Member

commented Feb 14, 2018

Squashed and merged.
Thanks for your contribution and patience with the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.