Quantcast

[pgAdmin4][Patch][RM_2400]: Columns with defaults set to NULL when removing contents after pasting in the edit grid

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[pgAdmin4][Patch][RM_2400]: Columns with defaults set to NULL when removing contents after pasting in the edit grid

Surinder Kumar
Hi Dave,

Implementation:

1) Took a flag 'is_row_copied' for each copied row:

 - to distinguish it from add/update row.
 - to write code specific to copied row only as it requires different logic than add row.

2) After pasting an existing row:

 - If a user clear the cell value, it must set cell to [default] value if default value is explicitly given for column while creating table otherwise [null].

 - Again, if a user entered a value in same cell, then removes that value, set it to [null] (the same behaviour is for add row).

3) Took a 2-dimensional array "grid.copied_rows" to keep track the changes of each row's cell so that changes made to each cell are independent and removed once data is saved.

4) On pasting a row, the cell must be highlighted with light green colour, so triggers an addNewRow event. Now copied row will add to grid instead of re-rendering all rows again.

5) Moved the common logic into functions.

This patch pass the feature test cases and jasmine test case.
Also I will add the test cases for copy row and send an updated patch.

Please find attached patch and review.


Thanks,
Surinder Kumar





--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

RM_2400.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [pgAdmin4][Patch][RM_2400]: Columns with defaults set to NULL when removing contents after pasting in the edit grid

Dave Page-7
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> wrote:
Hi Dave,

Implementation:

1) Took a flag 'is_row_copied' for each copied row:

 - to distinguish it from add/update row.
 - to write code specific to copied row only as it requires different logic than add row.

2) After pasting an existing row:

 - If a user clear the cell value, it must set cell to [default] value if default value is explicitly given for column while creating table otherwise [null].

 - Again, if a user entered a value in same cell, then removes that value, set it to [null] (the same behaviour is for add row).

3) Took a 2-dimensional array "grid.copied_rows" to keep track the changes of each row's cell so that changes made to each cell are independent and removed once data is saved.

4) On pasting a row, the cell must be highlighted with light green colour, so triggers an addNewRow event. Now copied row will add to grid instead of re-rendering all rows again.

5) Moved the common logic into functions.

This patch pass the feature test cases and jasmine test case.
Also I will add the test cases for copy row and send an updated patch.

Please find attached patch and review.

Looks good. I saw the following issues:

- The "new" row is not available once I've created the first new row, or pasted some.

- Rows are pasted in reverse order.

- If I copy/paste a new row that has yet to be saved, none of the values are actually copied.

- Attempting to save a row that contains all null/default values results in an invalid query:

INSERT INTO public.defaults (
) VALUES (
);

I think the only answer here is to explicitly insert NULL into any columns *without* a default value.

Thanks.
 
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [pgAdmin4][Patch][RM_2400]: Columns with defaults set to NULL when removing contents after pasting in the edit grid

Surinder Kumar
Hi Dave,

Please review the updated patch.

On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> wrote:
Hi Dave,

Implementation:

1) Took a flag 'is_row_copied' for each copied row:

 - to distinguish it from add/update row.
 - to write code specific to copied row only as it requires different logic than add row.

2) After pasting an existing row:

 - If a user clear the cell value, it must set cell to [default] value if default value is explicitly given for column while creating table otherwise [null].

 - Again, if a user entered a value in same cell, then removes that value, set it to [null] (the same behaviour is for add row).

3) Took a 2-dimensional array "grid.copied_rows" to keep track the changes of each row's cell so that changes made to each cell are independent and removed once data is saved.

4) On pasting a row, the cell must be highlighted with light green colour, so triggers an addNewRow event. Now copied row will add to grid instead of re-rendering all rows again.

5) Moved the common logic into functions.

This patch pass the feature test cases and jasmine test case.
Also I will add the test cases for copy row and send an updated patch.

Please find attached patch and review.

Looks good. I saw the following issues:

- The "new" row is not available once I've created the first new row, or pasted some.
​Fixed.

- Rows are pasted in reverse order.
​Fixed. 

- If I copy/paste a new row that has yet to be saved, none of the values are actually copied.
​Fixed.​

- Attempting to save a row that contains all null/default values results in an invalid query:

INSERT INTO public.defaults (
) VALUES (
);

I think the only answer here is to explicitly insert NULL into any columns *without* a default value.
​I could not reproduce, However  #3 might have fixed it too.​

Apart from this, I noticed epicRandomString(...) function doesn't generate unique number always, due to this save and delete rows was affected. Not all rows saved or deleted. The new function always returns a unique random number.
Fixed.

Thanks.
 
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

RM_2400_v1.patch (29K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [pgAdmin4][Patch][RM_2400]: Columns with defaults set to NULL when removing contents after pasting in the edit grid

Joao Pedro De Almeida Pereira-2
Hello Hackers,

We reviewed the PR, and there are a lot of new lines of code in this patch, are we sure that we can have a good coverage of the functionality just by doing feature tests?
Adding more code to a 3.5k+ lines file doesn't look like a good option, do you think it is possible to extract some of the functionality to their own files and have test around those functionalities?

Do we really need to have an epicRandomString function in our code? Would it be better to use a library that would provide us that functionality out of the box?
The functions this.applyValue in slick.pgadmin.editors.js that were change in this patch are exactly the same, can we extract that code into a single function instead of repeating the code?

Using feature tests is a good option to ensure that the integration of all the components of the application is working as expected, but in order to tests behaviors that are edge cases the Unit Tests are much cheaper to run and create.

Thanks
Joao & Shruti


On Thu, May 18, 2017 at 1:22 AM, Surinder Kumar <[hidden email]> wrote:
Hi Dave,

Please review the updated patch.

On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> wrote:
Hi Dave,

Implementation:

1) Took a flag 'is_row_copied' for each copied row:

 - to distinguish it from add/update row.
 - to write code specific to copied row only as it requires different logic than add row.

2) After pasting an existing row:

 - If a user clear the cell value, it must set cell to [default] value if default value is explicitly given for column while creating table otherwise [null].

 - Again, if a user entered a value in same cell, then removes that value, set it to [null] (the same behaviour is for add row).

3) Took a 2-dimensional array "grid.copied_rows" to keep track the changes of each row's cell so that changes made to each cell are independent and removed once data is saved.

4) On pasting a row, the cell must be highlighted with light green colour, so triggers an addNewRow event. Now copied row will add to grid instead of re-rendering all rows again.

5) Moved the common logic into functions.

This patch pass the feature test cases and jasmine test case.
Also I will add the test cases for copy row and send an updated patch.

Please find attached patch and review.

Looks good. I saw the following issues:

- The "new" row is not available once I've created the first new row, or pasted some.
​Fixed.

- Rows are pasted in reverse order.
​Fixed. 

- If I copy/paste a new row that has yet to be saved, none of the values are actually copied.
​Fixed.​

- Attempting to save a row that contains all null/default values results in an invalid query:

INSERT INTO public.defaults (
) VALUES (
);

I think the only answer here is to explicitly insert NULL into any columns *without* a default value.
​I could not reproduce, However  #3 might have fixed it too.​

Apart from this, I noticed epicRandomString(...) function doesn't generate unique number always, due to this save and delete rows was affected. Not all rows saved or deleted. The new function always returns a unique random number.
Fixed.

Thanks.
 
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [pgAdmin4][Patch][RM_2400]: Columns with defaults set to NULL when removing contents after pasting in the edit grid

Surinder Kumar
Hi

Please find updated patch and review.

On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira <[hidden email]> wrote:
Hello Hackers,

We reviewed the PR, and there are a lot of new lines of code in this patch, are we sure that we can have a good coverage of the functionality just by doing feature tests?
Adding more code to a 3.5k+ lines file doesn't look like a good option, do you think it is possible to extract some of the functionality to their own files and have test around those functionalities?​
​To improve the code readability, reduce code complexity and to make code testable, the code must be splitted component wise.
Here is my suggestion:

1. The code for classes like “SQLEditorView” and “SqlEditorController” can be moved into two files like "editor_view.js and “editor_controller.js" and called from within "sqleditor.js".

2. All utilities functions can be moved into separate utils file and can write test cases.

3. Slickgrid listener functions such as:
onBeforeEditCell, onKeyDown,
​ ​
onCellChange, onAddNewRow
​ 
can be moved into
​ a file and write test cases around.

This needs discussion. Any suggestion?

Do we really need to have an epicRandomString function in our code? Would it be better to use a library that would provide us that functionality out of the box?
We are using "epicRandomString function" to uniquely identify each record, I talked to Harshal who is eliminating the use of this function and instead maintaining counter for the rows added/updated/deleted. 
The functions this.applyValue in slick.pgadmin.editors.js that were change in this patch are exactly the same, can we extract that code into a single function instead of repeating the code?
​I have moved common logic into a new function.​

Using feature tests is a good option to ensure that the integration of all the components of the application is working as expected, but in order to tests behaviors that are edge cases the Unit Tests are much cheaper to run and create.
​I will write test cases for functions once they are moved into their separate files as discussed above.

Thanks
Joao & Shruti


On Thu, May 18, 2017 at 1:22 AM, Surinder Kumar <[hidden email]> wrote:
Hi Dave,

Please review the updated patch.

On Wed, May 17, 2017 at 8:46 PM, Dave Page <[hidden email]> wrote:
Hi

On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <[hidden email]> wrote:
Hi Dave,

Implementation:

1) Took a flag 'is_row_copied' for each copied row:

 - to distinguish it from add/update row.
 - to write code specific to copied row only as it requires different logic than add row.

2) After pasting an existing row:

 - If a user clear the cell value, it must set cell to [default] value if default value is explicitly given for column while creating table otherwise [null].

 - Again, if a user entered a value in same cell, then removes that value, set it to [null] (the same behaviour is for add row).

3) Took a 2-dimensional array "grid.copied_rows" to keep track the changes of each row's cell so that changes made to each cell are independent and removed once data is saved.

4) On pasting a row, the cell must be highlighted with light green colour, so triggers an addNewRow event. Now copied row will add to grid instead of re-rendering all rows again.

5) Moved the common logic into functions.

This patch pass the feature test cases and jasmine test case.
Also I will add the test cases for copy row and send an updated patch.

Please find attached patch and review.

Looks good. I saw the following issues:

- The "new" row is not available once I've created the first new row, or pasted some.
​Fixed.

- Rows are pasted in reverse order.
​Fixed. 

- If I copy/paste a new row that has yet to be saved, none of the values are actually copied.
​Fixed.​

- Attempting to save a row that contains all null/default values results in an invalid query:

INSERT INTO public.defaults (
) VALUES (
);

I think the only answer here is to explicitly insert NULL into any columns *without* a default value.
​I could not reproduce, However  #3 might have fixed it too.​

Apart from this, I noticed epicRandomString(...) function doesn't generate unique number always, due to this save and delete rows was affected. Not all rows saved or deleted. The new function always returns a unique random number.
Fixed.

Thanks.
 
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers





--
Sent via pgadmin-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

RM_2400_v2.patch (28K) Download Attachment
Previous Thread Next Thread
Loading...