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

Hi

Please find updated patch and review.

On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> 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 <
> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave,
>>
>> Please review the updated patch.
>>
>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar <
>>> surinder(dot)kumar(at)enterprisedb(dot)com> 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 (pgadmin-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>
>>
>