Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

On Fri, May 12, 2017 at 3:41 PM, Dave Page  wrote:

> Hi,
>
> Couple of comments:
>
> - Can we improve the speed of this test? Perhaps by adding multiple rows
> to the table at once, then checking the result after a single save/refresh?
> We need to keep the feature tests as fast as possible to ensure they remain
> practical to run.
>
​Sure. I will do this and send updated patch.​

>
> - I get the following failure under Python 2. It passes under Python 3 as
> you might imagine given the assertion error.
>
​I will check.​

>
> Thanks!
>
> 2017-05-12 11:00:00,860:ERROR:STDERR:==
> 
> 2017-05-12 11:00:00,861:ERROR:STDERR:FAIL: runTest
> (pgadmin.feature_tests.view_data_dml_queries.CheckForViewDataTest)
> 2017-05-12 11:00:00,861:ERROR:STDERR:Validate Insert, Update operations
> in View data with given test data
> 2017-05-12 11:00:00,861:ERROR:STDERR:--
> 
> 2017-05-12 11:00:00,861:ERROR:STDERR:Traceback (most recent call last):
> 2017-05-12 11:00:00,861:ERROR:STDERR:  File "/Users/dpage/git/pgadmin4/web
> /pgadmin/feature_tests/view_data_dml_queries.py", line 160, in runTest
> 2017-05-12 11:00:00,861:ERROR:STDERR:self._update_row_in_table(key)
> 2017-05-12 11:00:00,861:ERROR:STDERR:  File "/Users/dpage/git/pgadmin4/web
> /pgadmin/feature_tests/view_data_dml_queries.py", line 432, in
> _update_row_in_table
> 2017-05-12 11:00:00,861:ERROR:STDERR:self._verify_update_data(table,
> row)
> 2017-05-12 11:00:00,861:ERROR:STDERR:  File "/Users/dpage/git/pgadmin4/web
> /pgadmin/feature_tests/view_data_dml_queries.py", line 477, in
> _verify_update_data
> 2017-05-12 11:00:00,861:ERROR:STDERR:self.assertEquals(cell2,
> test_verify_data['data_default_nulls'])
> 2017-05-12 11:00:00,861:ERROR:STDERR:AssertionError: "''" != u''
> 2017-05-12 11:00:00,861:ERROR:STDERR:--
> 
> 2017-05-12 11:00:00,861:ERROR:STDERR:Ran 6 tests in 208.850s
> 2017-05-12 11:00:00,861:ERROR:STDERR:FAILED
> 2017-05-12 11:00:00,861:ERROR:STDERR: (failures=1)
> On Wed, May 10, 2017 at 3:02 PM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> Please find attached patch for Feature test cases for RM_2257
>>
>> *Implementation detail:*
>>
>>  - Added a test_data.json file which contains Insert and Update test
>> related input data
>>
>>  - First of all, we create three tables such as
>>  a) defaults_text
>>  b) defaults_boolean
>>  c) defaults_number
>>  d) defaults_json
>>  These tables has columns with different constraints (default value,
>> not_null etc) to test with various input test data.
>>
>> - Test cases for insert are executed first and then test cases for update.
>>
>> Please review the patch.
>>
>>
>> On Wed, May 10, 2017 at 2:22 PM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> Please find attached patch for RM only.
>>>
>>> *Changes:*
>>>
>>>  - All formatters now handles both [null] and [default] values
>>>
>>>  - the cell values are validated on server side as in pgAdmin3.
>>>
>>>  - added light grey color for cells with [null] and [default]
>>> placeholders.
>>>
>>> On Wed, May 10, 2017 at 2:12 PM, Dave Page  wrote:
>>>


 On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar <
 surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Wed, May 10, 2017 at 2:06 PM, Dave Page  wrote:
>
>> Any chance we can get this wrapped up today Surinder?
>>
> ​I have fixed RM case, I am currently writing its feature test cases
> which is taking some time.
> Should I send patch for RM case only?​ I will try to complete test
> cases by today eod.
>

 Yes please.

 Thanks!

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

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

>>>
>>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

Hi,

Couple of comments:

- Can we improve the speed of this test? Perhaps by adding multiple rows to
the table at once, then checking the result after a single save/refresh? We
need to keep the feature tests as fast as possible to ensure they remain
practical to run.

- I get the following failure under Python 2. It passes under Python 3 as
you might imagine given the assertion error.

Thanks!

2017-05-12 11:00:00,860:ERROR:STDERR:==

2017-05-12 11:00:00,861:ERROR:STDERR:FAIL: runTest
(pgadmin.feature_tests.view_data_dml_queries.CheckForViewDataTest)
2017-05-12 11:00:00,861:ERROR:STDERR:Validate Insert, Update operations in
View data with given test data
2017-05-12 11:00:00,861:ERROR:STDERR:--

2017-05-12 11:00:00,861:ERROR:STDERR:Traceback (most recent call last):
2017-05-12 11:00:00,861:ERROR:STDERR:  File "/Users/dpage/git/pgadmin4/
web/pgadmin/feature_tests/view_data_dml_queries.py", line 160, in runTest
2017-05-12 11:00:00,861:ERROR:STDERR:self._update_row_in_table(key)
2017-05-12 11:00:00,861:ERROR:STDERR:  File "/Users/dpage/git/pgadmin4/
web/pgadmin/feature_tests/view_data_dml_queries.py", line 432, in
_update_row_in_table
2017-05-12 11:00:00,861:ERROR:STDERR:self._verify_update_data(table,
row)
2017-05-12 11:00:00,861:ERROR:STDERR:  File "/Users/dpage/git/pgadmin4/
web/pgadmin/feature_tests/view_data_dml_queries.py", line 477, in
_verify_update_data
2017-05-12 11:00:00,861:ERROR:STDERR:self.assertEquals(cell2,
test_verify_data['data_default_nulls'])
2017-05-12 11:00:00,861:ERROR:STDERR:AssertionError: "''" != u''
2017-05-12 11:00:00,861:ERROR:STDERR:--

2017-05-12 11:00:00,861:ERROR:STDERR:Ran 6 tests in 208.850s
2017-05-12 11:00:00,861:ERROR:STDERR:FAILED
2017-05-12 11:00:00,861:ERROR:STDERR: (failures=1)
On Wed, May 10, 2017 at 3:02 PM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> Please find attached patch for Feature test cases for RM_2257
>
> *Implementation detail:*
>
>  - Added a test_data.json file which contains Insert and Update test
> related input data
>
>  - First of all, we create three tables such as
>  a) defaults_text
>  b) defaults_boolean
>  c) defaults_number
>  d) defaults_json
>  These tables has columns with different constraints (default value,
> not_null etc) to test with various input test data.
>
> - Test cases for insert are executed first and then test cases for update.
>
> Please review the patch.
>
>
> On Wed, May 10, 2017 at 2:22 PM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> Please find attached patch for RM only.
>>
>> *Changes:*
>>
>>  - All formatters now handles both [null] and [default] values
>>
>>  - the cell values are validated on server side as in pgAdmin3.
>>
>>  - added light grey color for cells with [null] and [default]
>> placeholders.
>>
>> On Wed, May 10, 2017 at 2:12 PM, Dave Page  wrote:
>>
>>>
>>>
>>> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar <
>>> surinder.ku...@enterprisedb.com> wrote:
>>>
 Hi Dave,

 On Wed, May 10, 2017 at 2:06 PM, Dave Page  wrote:

> Any chance we can get this wrapped up today Surinder?
>
 ​I have fixed RM case, I am currently writing its feature test cases
 which is taking some time.
 Should I send patch for RM case only?​ I will try to complete test
 cases by today eod.

>>>
>>> Yes please.
>>>
>>> Thanks!
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>


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

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

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

Please find updated patch

On Thu, May 11, 2017 at 2:29 PM, Dave Page  wrote:

> Ooops, managed to hit send too soon.
>
> ... it leads to failed updates such as:
>
> 2017-05-11 09:55:47,570: SQL pgadmin: Execute (void) for server #1 -
> CONN:2096775 (Query-id: 4540472):
> UPDATE public.defaults SET
> data_default_no_nulls = 'asas' WHERE
> ;
> 2017-05-11 09:55:47,577: ERROR pgadmin:
> Failed to execute query (execute_void) for the server #1 - CONN:2096775
> (Query-id: 4540472):
> Error Message:ERROR:  syntax error at or near ";"
> LINE 3: ;
>
​The new rows added is kept disabled untill grid is not refreshed.

>
>
> On Thu, May 11, 2017 at 9:58 AM, Dave Page  wrote:
>
>> Hi
>>
>> There seems to be couple of bugs in this;
>>
>> - When creating a new row with my test table, if I click in the id
>> column, don't change anything, then click in another column, the ID column
>> value changes from [default] to [null], making it impossible to save that
>> row with the default value. In this case I would expect it to stay at
>> [default] unless I explicitly entered a value.
>>
> ​Fixed.​

>
>> - When I add a new row, but leave the id as [default], the row is saved,
>> but the [default] marker changes from gray to black (but only in the id
>> column.
>>
> ​I forgot to add 'grey_color' class for numeric type fields. Now added.​

>
>> - I'm able to edit a freshly added row immediately after saving but
>> before refreshing. This shouldn't be allowed if we don't know what the
>> primary key value is, as it leads to failed updates such as:
>>
>>
>> On Wed, May 10, 2017 at 9:52 AM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> Please find attached patch for RM only.
>>>
>>> *Changes:*
>>>
>>>  - All formatters now handles both [null] and [default] values
>>>
>>>  - the cell values are validated on server side as in pgAdmin3.
>>>
>>>  - added light grey color for cells with [null] and [default]
>>> placeholders.
>>>
>>> On Wed, May 10, 2017 at 2:12 PM, Dave Page  wrote:
>>>


 On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar <
 surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Wed, May 10, 2017 at 2:06 PM, Dave Page  wrote:
>
>> Any chance we can get this wrapped up today Surinder?
>>
> ​I have fixed RM case, I am currently writing its feature test cases
> which is taking some time.
> Should I send patch for RM case only?​ I will try to complete test
> cases by today eod.
>

 Yes please.

 Thanks!

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

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

>>>
>>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


RM_2257_v4.patch
Description: Binary data

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

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

Hi

There seems to be couple of bugs in this;

- When creating a new row with my test table, if I click in the id column,
don't change anything, then click in another column, the ID column value
changes from [default] to [null], making it impossible to save that row
with the default value. In this case I would expect it to stay at [default]
unless I explicitly entered a value.

- When I add a new row, but leave the id as [default], the row is saved,
but the [default] marker changes from gray to black (but only in the id
column.

- I'm able to edit a freshly added row immediately after saving but before
refreshing. This shouldn't be allowed if we don't know what the primary key
value is, as it leads to failed updates such as:


On Wed, May 10, 2017 at 9:52 AM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> Please find attached patch for RM only.
>
> *Changes:*
>
>  - All formatters now handles both [null] and [default] values
>
>  - the cell values are validated on server side as in pgAdmin3.
>
>  - added light grey color for cells with [null] and [default] placeholders.
>
> On Wed, May 10, 2017 at 2:12 PM, Dave Page  wrote:
>
>>
>>
>> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Wed, May 10, 2017 at 2:06 PM, Dave Page  wrote:
>>>
 Any chance we can get this wrapped up today Surinder?

>>> ​I have fixed RM case, I am currently writing its feature test cases
>>> which is taking some time.
>>> Should I send patch for RM case only?​ I will try to complete test cases
>>> by today eod.
>>>
>>
>> Yes please.
>>
>> Thanks!
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>


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

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

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

Please find attached patch for Feature test cases for RM_2257

*Implementation detail:*

 - Added a test_data.json file which contains Insert and Update test
related input data

 - First of all, we create three tables such as
 a) defaults_text
 b) defaults_boolean
 c) defaults_number
 d) defaults_json
 These tables has columns with different constraints (default value,
not_null etc) to test with various input test data.

- Test cases for insert are executed first and then test cases for update.

Please review the patch.


On Wed, May 10, 2017 at 2:22 PM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> Please find attached patch for RM only.
>
> *Changes:*
>
>  - All formatters now handles both [null] and [default] values
>
>  - the cell values are validated on server side as in pgAdmin3.
>
>  - added light grey color for cells with [null] and [default] placeholders.
>
> On Wed, May 10, 2017 at 2:12 PM, Dave Page  wrote:
>
>>
>>
>> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Wed, May 10, 2017 at 2:06 PM, Dave Page  wrote:
>>>
 Any chance we can get this wrapped up today Surinder?

>>> ​I have fixed RM case, I am currently writing its feature test cases
>>> which is taking some time.
>>> Should I send patch for RM case only?​ I will try to complete test cases
>>> by today eod.
>>>
>>
>> Yes please.
>>
>> Thanks!
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>


features_test_cases_RM_2257_v1.patch
Description: Binary data

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

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

Please find attached patch for RM only.

*Changes:*

 - All formatters now handles both [null] and [default] values

 - the cell values are validated on server side as in pgAdmin3.

 - added light grey color for cells with [null] and [default] placeholders.

On Wed, May 10, 2017 at 2:12 PM, Dave Page  wrote:

>
>
> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> On Wed, May 10, 2017 at 2:06 PM, Dave Page  wrote:
>>
>>> Any chance we can get this wrapped up today Surinder?
>>>
>> ​I have fixed RM case, I am currently writing its feature test cases
>> which is taking some time.
>> Should I send patch for RM case only?​ I will try to complete test cases
>> by today eod.
>>
>
> Yes please.
>
> Thanks!
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


RM_2257_v3.patch
Description: Binary data

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

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Wed, May 10, 2017 at 2:06 PM, Dave Page  wrote:
>
>> Any chance we can get this wrapped up today Surinder?
>>
> ​I have fixed RM case, I am currently writing its feature test cases which
> is taking some time.
> Should I send patch for RM case only?​ I will try to complete test cases
> by today eod.
>

Yes please.

Thanks!

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

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

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

On Wed, May 10, 2017 at 2:06 PM, Dave Page  wrote:

> Any chance we can get this wrapped up today Surinder?
>
​I have fixed RM case, I am currently writing its feature test cases which
is taking some time.
Should I send patch for RM case only?​ I will try to complete test cases by
today eod.

>
> On Tue, May 9, 2017 at 11:29 AM, Dave Page  wrote:
>
>>
>>
>> On Tue, May 9, 2017 at 11:03 AM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Mon, May 8, 2017 at 4:37 PM, Dave Page  wrote:
>>>


 On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar <
 surinder.ku...@enterprisedb.com> wrote:

> Hi
>
> On Mon, May 8, 2017 at 3:51 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Mon, May 8, 2017 at 3:28 PM, Dave Page  wrote:
>>>
 Hi

 On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
 surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> The support to handle [null] and [default] values is added for
> following formatters:
>  - JsonFormatter
>  - NumbersFormatter
>  - CheckmarkFormatter
>  - TextFormatter
>
> Previously when a new row is added, it was not validating each and
> every cell for columns with attribute not_null = true.
> Introduced a new function validateRow(...) which is called on
> adding a new row, it validates each cell with column having
> attribute(not_null=true). the corresponding cell will highlighted and 
> save
> button will be disabled if value is [null].
>

 I'm not sure that behaviour is right. What I now see (given the
 table below) is that:

 - If I click in the id field of a new row, it forces me to enter a
 value or hit escape. Why is it trying to force me? It's a serial 
 field, so
 will get a value on save anyway.

>>> ​Yes, It is because I am validating all cell values after it
>>> renders. It will reverted back.
>>>

 - If I do enter a value in the ID field, focus jumps over all the
 other fields with either a default or no 'not null' option to the
 data_no_nulls column. Focus should always go to the next field.

 I think this addition can just be removed - I'm pretty sure the
 previous behaviour would have been what we want, with the additional
 formatters fixed.

>>> ​But If i remove this addition, the value for column like
>>> 'data_no_nulls' ​will be set to '' (blank string), then on save its 
>>> value
>>> will be validated on the server side and whatever the error message is 
>>> will
>>> be returned back (the same behaviour as in pgAdmin3)
>>>
>>
>> Which is fine I think. If you want to leave the validation there,
>> that's also fine - but, it a) shouldn't require me to press Esc if I 
>> decide
>> not to fill in that value yet, and b) shouldn't change the tab order of 
>> the
>> fields. It's also broken of course, in that it was trying to force me to
>> enter a value for a not-null field with a default.
>>
> ​Yes, I will check if we just highlight the field and don't force the
> user to ​enter value.
> This will enable user to edit any field using TAB key even if required
> field is highlighted red.
> Should the save button remains disable untill user enters any valid
> value in 'data_no_nulls' column ?
>

 I think that's fine, yes - as long as we get the validation right :-)

>>> ​For highlighting the error field and enable user to navigate to other
>>> cells using TAB key,
>>> I spend some time looking and debugging into the code and found that it
>>> requires lot of changes in slick.grid.js core functions as there are no
>>> event listeners provided to ​listen and changes in core file is not
>>> preferred. It may also break other functionalities as code is quite complex.
>>> So, I think we should validate data on server side and display any error
>>> messages on UI.
>>>
>>
>> OK.
>>
>>
>>>
>>
>

>
> Now I will add more feature test cases for remaining formatters.
> Will send separate patch for feature test cases once completed.
>

 Thanks.


>
> Please review updated patch.
>
>
> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> On Tue, May 2, 2017 at 5:21 PM, Dave Page 
>> wrote:

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

Any chance we can get this wrapped up today Surinder?

On Tue, May 9, 2017 at 11:29 AM, Dave Page  wrote:

>
>
> On Tue, May 9, 2017 at 11:03 AM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> On Mon, May 8, 2017 at 4:37 PM, Dave Page  wrote:
>>
>>>
>>>
>>> On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar <
>>> surinder.ku...@enterprisedb.com> wrote:
>>>
 Hi

 On Mon, May 8, 2017 at 3:51 PM, Dave Page  wrote:

> Hi
>
> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> On Mon, May 8, 2017 at 3:28 PM, Dave Page  wrote:
>>
>>> Hi
>>>
>>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
>>> surinder.ku...@enterprisedb.com> wrote:
>>>
 Hi Dave,

 The support to handle [null] and [default] values is added for
 following formatters:
  - JsonFormatter
  - NumbersFormatter
  - CheckmarkFormatter
  - TextFormatter

 Previously when a new row is added, it was not validating each and
 every cell for columns with attribute not_null = true.
 Introduced a new function validateRow(...) which is called on
 adding a new row, it validates each cell with column having
 attribute(not_null=true). the corresponding cell will highlighted and 
 save
 button will be disabled if value is [null].

>>>
>>> I'm not sure that behaviour is right. What I now see (given the
>>> table below) is that:
>>>
>>> - If I click in the id field of a new row, it forces me to enter a
>>> value or hit escape. Why is it trying to force me? It's a serial field, 
>>> so
>>> will get a value on save anyway.
>>>
>> ​Yes, It is because I am validating all cell values after it renders.
>> It will reverted back.
>>
>>>
>>> - If I do enter a value in the ID field, focus jumps over all the
>>> other fields with either a default or no 'not null' option to the
>>> data_no_nulls column. Focus should always go to the next field.
>>>
>>> I think this addition can just be removed - I'm pretty sure the
>>> previous behaviour would have been what we want, with the additional
>>> formatters fixed.
>>>
>> ​But If i remove this addition, the value for column like
>> 'data_no_nulls' ​will be set to '' (blank string), then on save its value
>> will be validated on the server side and whatever the error message is 
>> will
>> be returned back (the same behaviour as in pgAdmin3)
>>
>
> Which is fine I think. If you want to leave the validation there,
> that's also fine - but, it a) shouldn't require me to press Esc if I 
> decide
> not to fill in that value yet, and b) shouldn't change the tab order of 
> the
> fields. It's also broken of course, in that it was trying to force me to
> enter a value for a not-null field with a default.
>
 ​Yes, I will check if we just highlight the field and don't force the
 user to ​enter value.
 This will enable user to edit any field using TAB key even if required
 field is highlighted red.
 Should the save button remains disable untill user enters any valid
 value in 'data_no_nulls' column ?

>>>
>>> I think that's fine, yes - as long as we get the validation right :-)
>>>
>> ​For highlighting the error field and enable user to navigate to other
>> cells using TAB key,
>> I spend some time looking and debugging into the code and found that it
>> requires lot of changes in slick.grid.js core functions as there are no
>> event listeners provided to ​listen and changes in core file is not
>> preferred. It may also break other functionalities as code is quite complex.
>> So, I think we should validate data on server side and display any error
>> messages on UI.
>>
>
> OK.
>
>
>>
>

>>>

 Now I will add more feature test cases for remaining formatters.
 Will send separate patch for feature test cases once completed.

>>>
>>> Thanks.
>>>
>>>

 Please review updated patch.


 On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
 surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Tue, May 2, 2017 at 5:21 PM, Dave Page 
> wrote:
>
>> Hi,
>>
>> This is looking much better now :-). Couple of thoughts and a bug:
>>
>> - Only the TextFormatter seems to handle both [null] and
>> [default] values. Shouldn't all formatters do so (including Json and
>> Checkmark)?
>>
> ​Yes, I will apply the same changes for other formatters too.​
>
>> For example, 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

On Tue, May 9, 2017 at 11:03 AM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Mon, May 8, 2017 at 4:37 PM, Dave Page  wrote:
>
>>
>>
>> On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi
>>>
>>> On Mon, May 8, 2017 at 3:51 PM, Dave Page  wrote:
>>>
 Hi

 On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar <
 surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Mon, May 8, 2017 at 3:28 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> The support to handle [null] and [default] values is added for
>>> following formatters:
>>>  - JsonFormatter
>>>  - NumbersFormatter
>>>  - CheckmarkFormatter
>>>  - TextFormatter
>>>
>>> Previously when a new row is added, it was not validating each and
>>> every cell for columns with attribute not_null = true.
>>> Introduced a new function validateRow(...) which is called on
>>> adding a new row, it validates each cell with column having
>>> attribute(not_null=true). the corresponding cell will highlighted and 
>>> save
>>> button will be disabled if value is [null].
>>>
>>
>> I'm not sure that behaviour is right. What I now see (given the table
>> below) is that:
>>
>> - If I click in the id field of a new row, it forces me to enter a
>> value or hit escape. Why is it trying to force me? It's a serial field, 
>> so
>> will get a value on save anyway.
>>
> ​Yes, It is because I am validating all cell values after it renders.
> It will reverted back.
>
>>
>> - If I do enter a value in the ID field, focus jumps over all the
>> other fields with either a default or no 'not null' option to the
>> data_no_nulls column. Focus should always go to the next field.
>>
>> I think this addition can just be removed - I'm pretty sure the
>> previous behaviour would have been what we want, with the additional
>> formatters fixed.
>>
> ​But If i remove this addition, the value for column like
> 'data_no_nulls' ​will be set to '' (blank string), then on save its value
> will be validated on the server side and whatever the error message is 
> will
> be returned back (the same behaviour as in pgAdmin3)
>

 Which is fine I think. If you want to leave the validation there,
 that's also fine - but, it a) shouldn't require me to press Esc if I decide
 not to fill in that value yet, and b) shouldn't change the tab order of the
 fields. It's also broken of course, in that it was trying to force me to
 enter a value for a not-null field with a default.

>>> ​Yes, I will check if we just highlight the field and don't force the
>>> user to ​enter value.
>>> This will enable user to edit any field using TAB key even if required
>>> field is highlighted red.
>>> Should the save button remains disable untill user enters any valid
>>> value in 'data_no_nulls' column ?
>>>
>>
>> I think that's fine, yes - as long as we get the validation right :-)
>>
> ​For highlighting the error field and enable user to navigate to other
> cells using TAB key,
> I spend some time looking and debugging into the code and found that it
> requires lot of changes in slick.grid.js core functions as there are no
> event listeners provided to ​listen and changes in core file is not
> preferred. It may also break other functionalities as code is quite complex.
> So, I think we should validate data on server side and display any error
> messages on UI.
>

OK.


>

>>>
>>
>>>
>>> Now I will add more feature test cases for remaining formatters.
>>> Will send separate patch for feature test cases once completed.
>>>
>>
>> Thanks.
>>
>>
>>>
>>> Please review updated patch.
>>>
>>>
>>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
>>> surinder.ku...@enterprisedb.com> wrote:
>>>
 Hi Dave,

 On Tue, May 2, 2017 at 5:21 PM, Dave Page 
 wrote:

> Hi,
>
> This is looking much better now :-). Couple of thoughts and a bug:
>
> - Only the TextFormatter seems to handle both [null] and [default]
> values. Shouldn't all formatters do so (including Json and Checkmark)?
>
 ​Yes, I will apply the same changes for other formatters too.​

> For example, "serial" columns currently get displayed as [null]
> when left blank, but I would expect to see [default].
>
> - I would suggest we put [null] and [default] in a lighter colour
> - #99.
>
> - With the feature test patch added, I seem to be 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

On Mon, May 8, 2017 at 4:37 PM, Dave Page  wrote:

>
>
> On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi
>>
>> On Mon, May 8, 2017 at 3:51 PM, Dave Page  wrote:
>>
>>> Hi
>>>
>>> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar <
>>> surinder.ku...@enterprisedb.com> wrote:
>>>
 Hi Dave,

 On Mon, May 8, 2017 at 3:28 PM, Dave Page  wrote:

> Hi
>
> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> The support to handle [null] and [default] values is added for
>> following formatters:
>>  - JsonFormatter
>>  - NumbersFormatter
>>  - CheckmarkFormatter
>>  - TextFormatter
>>
>> Previously when a new row is added, it was not validating each and
>> every cell for columns with attribute not_null = true.
>> Introduced a new function validateRow(...) which is called on adding
>> a new row, it validates each cell with column having
>> attribute(not_null=true). the corresponding cell will highlighted and 
>> save
>> button will be disabled if value is [null].
>>
>
> I'm not sure that behaviour is right. What I now see (given the table
> below) is that:
>
> - If I click in the id field of a new row, it forces me to enter a
> value or hit escape. Why is it trying to force me? It's a serial field, so
> will get a value on save anyway.
>
 ​Yes, It is because I am validating all cell values after it renders.
 It will reverted back.

>
> - If I do enter a value in the ID field, focus jumps over all the
> other fields with either a default or no 'not null' option to the
> data_no_nulls column. Focus should always go to the next field.
>
> I think this addition can just be removed - I'm pretty sure the
> previous behaviour would have been what we want, with the additional
> formatters fixed.
>
 ​But If i remove this addition, the value for column like
 'data_no_nulls' ​will be set to '' (blank string), then on save its value
 will be validated on the server side and whatever the error message is will
 be returned back (the same behaviour as in pgAdmin3)

>>>
>>> Which is fine I think. If you want to leave the validation there, that's
>>> also fine - but, it a) shouldn't require me to press Esc if I decide not to
>>> fill in that value yet, and b) shouldn't change the tab order of the
>>> fields. It's also broken of course, in that it was trying to force me to
>>> enter a value for a not-null field with a default.
>>>
>> ​Yes, I will check if we just highlight the field and don't force the
>> user to ​enter value.
>> This will enable user to edit any field using TAB key even if required
>> field is highlighted red.
>> Should the save button remains disable untill user enters any valid value
>> in 'data_no_nulls' column ?
>>
>
> I think that's fine, yes - as long as we get the validation right :-)
>
​For highlighting the error field and enable user to navigate to other
cells using TAB key,
I spend some time looking and debugging into the code and found that it
requires lot of changes in slick.grid.js core functions as there are no
event listeners provided to ​listen and changes in core file is not
preferred. It may also break other functionalities as code is quite complex.
So, I think we should validate data on server side and display any error
messages on UI.

>
>>>
>>
>
>>
>> Now I will add more feature test cases for remaining formatters. Will
>> send separate patch for feature test cases once completed.
>>
>
> Thanks.
>
>
>>
>> Please review updated patch.
>>
>>
>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page  wrote:
>>>
 Hi,

 This is looking much better now :-). Couple of thoughts and a bug:

 - Only the TextFormatter seems to handle both [null] and [default]
 values. Shouldn't all formatters do so (including Json and Checkmark)?

>>> ​Yes, I will apply the same changes for other formatters too.​
>>>
 For example, "serial" columns currently get displayed as [null]
 when left blank, but I would expect to see [default].

 - I would suggest we put [null] and [default] in a lighter colour -
 #99.

 - With the feature test patch added, I seem to be consistently
 getting the following failure (immediately after your new tests run):

 
 ==
 ERROR: runTest (pgadmin.feature_tests.xss_che
 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi
>
> On Mon, May 8, 2017 at 3:51 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Mon, May 8, 2017 at 3:28 PM, Dave Page  wrote:
>>>
 Hi

 On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
 surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> The support to handle [null] and [default] values is added for
> following formatters:
>  - JsonFormatter
>  - NumbersFormatter
>  - CheckmarkFormatter
>  - TextFormatter
>
> Previously when a new row is added, it was not validating each and
> every cell for columns with attribute not_null = true.
> Introduced a new function validateRow(...) which is called on adding
> a new row, it validates each cell with column having
> attribute(not_null=true). the corresponding cell will highlighted and save
> button will be disabled if value is [null].
>

 I'm not sure that behaviour is right. What I now see (given the table
 below) is that:

 - If I click in the id field of a new row, it forces me to enter a
 value or hit escape. Why is it trying to force me? It's a serial field, so
 will get a value on save anyway.

>>> ​Yes, It is because I am validating all cell values after it renders. It
>>> will reverted back.
>>>

 - If I do enter a value in the ID field, focus jumps over all the other
 fields with either a default or no 'not null' option to the data_no_nulls
 column. Focus should always go to the next field.

 I think this addition can just be removed - I'm pretty sure the
 previous behaviour would have been what we want, with the additional
 formatters fixed.

>>> ​But If i remove this addition, the value for column like
>>> 'data_no_nulls' ​will be set to '' (blank string), then on save its value
>>> will be validated on the server side and whatever the error message is will
>>> be returned back (the same behaviour as in pgAdmin3)
>>>
>>
>> Which is fine I think. If you want to leave the validation there, that's
>> also fine - but, it a) shouldn't require me to press Esc if I decide not to
>> fill in that value yet, and b) shouldn't change the tab order of the
>> fields. It's also broken of course, in that it was trying to force me to
>> enter a value for a not-null field with a default.
>>
> ​Yes, I will check if we just highlight the field and don't force the user
> to ​enter value.
> This will enable user to edit any field using TAB key even if required
> field is highlighted red.
> Should the save button remains disable untill user enters any valid value
> in 'data_no_nulls' column ?
>

I think that's fine, yes - as long as we get the validation right :-)



>
>>
>

>
> Now I will add more feature test cases for remaining formatters. Will
> send separate patch for feature test cases once completed.
>

 Thanks.


>
> Please review updated patch.
>
>
> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> On Tue, May 2, 2017 at 5:21 PM, Dave Page  wrote:
>>
>>> Hi,
>>>
>>> This is looking much better now :-). Couple of thoughts and a bug:
>>>
>>> - Only the TextFormatter seems to handle both [null] and [default]
>>> values. Shouldn't all formatters do so (including Json and Checkmark)?
>>>
>> ​Yes, I will apply the same changes for other formatters too.​
>>
>>> For example, "serial" columns currently get displayed as [null] when
>>> left blank, but I would expect to see [default].
>>>
>>> - I would suggest we put [null] and [default] in a lighter colour -
>>> #99.
>>>
>>> - With the feature test patch added, I seem to be consistently
>>> getting the following failure (immediately after your new tests run):
>>>
>>> 
>>> ==
>>> ERROR: runTest (pgadmin.feature_tests.xss_che
>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest)
>>> Test XSS check for panels and query tool
>>> 
>>> --
>>> Traceback (most recent call last):
>>>   File 
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>>> line 42, in setUp
>>> self._screenshot()
>>>   File 
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>>> line 92, in _screenshot
>>> python_version))
>>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi

On Mon, May 8, 2017 at 3:51 PM, Dave Page  wrote:

> Hi
>
> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> On Mon, May 8, 2017 at 3:28 PM, Dave Page  wrote:
>>
>>> Hi
>>>
>>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
>>> surinder.ku...@enterprisedb.com> wrote:
>>>
 Hi Dave,

 The support to handle [null] and [default] values is added for
 following formatters:
  - JsonFormatter
  - NumbersFormatter
  - CheckmarkFormatter
  - TextFormatter

 Previously when a new row is added, it was not validating each and
 every cell for columns with attribute not_null = true.
 Introduced a new function validateRow(...) which is called on adding a
 new row, it validates each cell with column having
 attribute(not_null=true). the corresponding cell will highlighted and save
 button will be disabled if value is [null].

>>>
>>> I'm not sure that behaviour is right. What I now see (given the table
>>> below) is that:
>>>
>>> - If I click in the id field of a new row, it forces me to enter a value
>>> or hit escape. Why is it trying to force me? It's a serial field, so will
>>> get a value on save anyway.
>>>
>> ​Yes, It is because I am validating all cell values after it renders. It
>> will reverted back.
>>
>>>
>>> - If I do enter a value in the ID field, focus jumps over all the other
>>> fields with either a default or no 'not null' option to the data_no_nulls
>>> column. Focus should always go to the next field.
>>>
>>> I think this addition can just be removed - I'm pretty sure the previous
>>> behaviour would have been what we want, with the additional formatters
>>> fixed.
>>>
>> ​But If i remove this addition, the value for column like 'data_no_nulls'
>> ​will be set to '' (blank string), then on save its value will be validated
>> on the server side and whatever the error message is will be returned back
>> (the same behaviour as in pgAdmin3)
>>
>
> Which is fine I think. If you want to leave the validation there, that's
> also fine - but, it a) shouldn't require me to press Esc if I decide not to
> fill in that value yet, and b) shouldn't change the tab order of the
> fields. It's also broken of course, in that it was trying to force me to
> enter a value for a not-null field with a default.
>
​Yes, I will check if we just highlight the field and don't force the user
to ​enter value.
This will enable user to edit any field using TAB key even if required
field is highlighted red.
Should the save button remains disable untill user enters any valid value
in 'data_no_nulls' column ?

>
>

>>>

 Now I will add more feature test cases for remaining formatters. Will
 send separate patch for feature test cases once completed.

>>>
>>> Thanks.
>>>
>>>

 Please review updated patch.


 On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
 surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Tue, May 2, 2017 at 5:21 PM, Dave Page  wrote:
>
>> Hi,
>>
>> This is looking much better now :-). Couple of thoughts and a bug:
>>
>> - Only the TextFormatter seems to handle both [null] and [default]
>> values. Shouldn't all formatters do so (including Json and Checkmark)?
>>
> ​Yes, I will apply the same changes for other formatters too.​
>
>> For example, "serial" columns currently get displayed as [null] when
>> left blank, but I would expect to see [default].
>>
>> - I would suggest we put [null] and [default] in a lighter colour -
>> #99.
>>
>> - With the feature test patch added, I seem to be consistently
>> getting the following failure (immediately after your new tests run):
>>
>> 
>> ==
>> ERROR: runTest (pgadmin.feature_tests.xss_che
>> cks_panels_and_query_tool_test.CheckForXssFeatureTest)
>> Test XSS check for panels and query tool
>> 
>> --
>> Traceback (most recent call last):
>>   File 
>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>> line 42, in setUp
>> self._screenshot()
>>   File 
>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>> line 92, in _screenshot
>> python_version))
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 802, in
>> get_screenshot_as_file
>> png = self.get_screenshot_as_png()
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 821, in
>> get_screenshot_as_png
>> return base64.b64decode(self.get_scre
>> 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

Hi

On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Mon, May 8, 2017 at 3:28 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> The support to handle [null] and [default] values is added for following
>>> formatters:
>>>  - JsonFormatter
>>>  - NumbersFormatter
>>>  - CheckmarkFormatter
>>>  - TextFormatter
>>>
>>> Previously when a new row is added, it was not validating each and every
>>> cell for columns with attribute not_null = true.
>>> Introduced a new function validateRow(...) which is called on adding a
>>> new row, it validates each cell with column having
>>> attribute(not_null=true). the corresponding cell will highlighted and save
>>> button will be disabled if value is [null].
>>>
>>
>> I'm not sure that behaviour is right. What I now see (given the table
>> below) is that:
>>
>> - If I click in the id field of a new row, it forces me to enter a value
>> or hit escape. Why is it trying to force me? It's a serial field, so will
>> get a value on save anyway.
>>
> ​Yes, It is because I am validating all cell values after it renders. It
> will reverted back.
>
>>
>> - If I do enter a value in the ID field, focus jumps over all the other
>> fields with either a default or no 'not null' option to the data_no_nulls
>> column. Focus should always go to the next field.
>>
>> I think this addition can just be removed - I'm pretty sure the previous
>> behaviour would have been what we want, with the additional formatters
>> fixed.
>>
> ​But If i remove this addition, the value for column like 'data_no_nulls'
> ​will be set to '' (blank string), then on save its value will be validated
> on the server side and whatever the error message is will be returned back
> (the same behaviour as in pgAdmin3)
>

Which is fine I think. If you want to leave the validation there, that's
also fine - but, it a) shouldn't require me to press Esc if I decide not to
fill in that value yet, and b) shouldn't change the tab order of the
fields. It's also broken of course, in that it was trying to force me to
enter a value for a not-null field with a default.


>
>>
>>>
>>> Now I will add more feature test cases for remaining formatters. Will
>>> send separate patch for feature test cases once completed.
>>>
>>
>> Thanks.
>>
>>
>>>
>>> Please review updated patch.
>>>
>>>
>>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
>>> surinder.ku...@enterprisedb.com> wrote:
>>>
 Hi Dave,

 On Tue, May 2, 2017 at 5:21 PM, Dave Page  wrote:

> Hi,
>
> This is looking much better now :-). Couple of thoughts and a bug:
>
> - Only the TextFormatter seems to handle both [null] and [default]
> values. Shouldn't all formatters do so (including Json and Checkmark)?
>
 ​Yes, I will apply the same changes for other formatters too.​

> For example, "serial" columns currently get displayed as [null] when
> left blank, but I would expect to see [default].
>
> - I would suggest we put [null] and [default] in a lighter colour -
> #99.
>
> - With the feature test patch added, I seem to be consistently getting
> the following failure (immediately after your new tests run):
>
> ==
> ERROR: runTest (pgadmin.feature_tests.xss_che
> cks_panels_and_query_tool_test.CheckForXssFeatureTest)
> Test XSS check for panels and query tool
> --
> Traceback (most recent call last):
>   File 
> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
> line 42, in setUp
> self._screenshot()
>   File 
> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
> line 92, in _screenshot
> python_version))
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
> ges/selenium/webdriver/remote/webdriver.py", line 802, in
> get_screenshot_as_file
> png = self.get_screenshot_as_png()
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
> ges/selenium/webdriver/remote/webdriver.py", line 821, in
> get_screenshot_as_png
> return base64.b64decode(self.get_scre
> enshot_as_base64().encode('ascii'))
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
> ges/selenium/webdriver/remote/webdriver.py", line 831, in
> get_screenshot_as_base64
> return self.execute(Command.SCREENSHOT)['value']
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute
> self.error_handler.check_response(response)
>   File 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

On Mon, May 8, 2017 at 3:28 PM, Dave Page  wrote:

> Hi
>
> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> The support to handle [null] and [default] values is added for following
>> formatters:
>>  - JsonFormatter
>>  - NumbersFormatter
>>  - CheckmarkFormatter
>>  - TextFormatter
>>
>> Previously when a new row is added, it was not validating each and every
>> cell for columns with attribute not_null = true.
>> Introduced a new function validateRow(...) which is called on adding a
>> new row, it validates each cell with column having
>> attribute(not_null=true). the corresponding cell will highlighted and save
>> button will be disabled if value is [null].
>>
>
> I'm not sure that behaviour is right. What I now see (given the table
> below) is that:
>
> - If I click in the id field of a new row, it forces me to enter a value
> or hit escape. Why is it trying to force me? It's a serial field, so will
> get a value on save anyway.
>
​Yes, It is because I am validating all cell values after it renders. It
will reverted back.

>
> - If I do enter a value in the ID field, focus jumps over all the other
> fields with either a default or no 'not null' option to the data_no_nulls
> column. Focus should always go to the next field.
>
> I think this addition can just be removed - I'm pretty sure the previous
> behaviour would have been what we want, with the additional formatters
> fixed.
>
​But If i remove this addition, the value for column like 'data_no_nulls'
​will be set to '' (blank string), then on save its value will be validated
on the server side and whatever the error message is will be returned back
(the same behaviour as in pgAdmin3)

>
>
>>
>> Now I will add more feature test cases for remaining formatters. Will
>> send separate patch for feature test cases once completed.
>>
>
> Thanks.
>
>
>>
>> Please review updated patch.
>>
>>
>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
>> surinder.ku...@enterprisedb.com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page  wrote:
>>>
 Hi,

 This is looking much better now :-). Couple of thoughts and a bug:

 - Only the TextFormatter seems to handle both [null] and [default]
 values. Shouldn't all formatters do so (including Json and Checkmark)?

>>> ​Yes, I will apply the same changes for other formatters too.​
>>>
 For example, "serial" columns currently get displayed as [null] when
 left blank, but I would expect to see [default].

 - I would suggest we put [null] and [default] in a lighter colour -
 #99.

 - With the feature test patch added, I seem to be consistently getting
 the following failure (immediately after your new tests run):

 ==
 ERROR: runTest (pgadmin.feature_tests.xss_che
 cks_panels_and_query_tool_test.CheckForXssFeatureTest)
 Test XSS check for panels and query tool
 --
 Traceback (most recent call last):
   File 
 "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
 line 42, in setUp
 self._screenshot()
   File 
 "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
 line 92, in _screenshot
 python_version))
   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
 ges/selenium/webdriver/remote/webdriver.py", line 802, in
 get_screenshot_as_file
 png = self.get_screenshot_as_png()
   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
 ges/selenium/webdriver/remote/webdriver.py", line 821, in
 get_screenshot_as_png
 return base64.b64decode(self.get_screenshot_as_base64().encode('asc
 ii'))
   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
 ges/selenium/webdriver/remote/webdriver.py", line 831, in
 get_screenshot_as_base64
 return self.execute(Command.SCREENSHOT)['value']
   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
 ges/selenium/webdriver/remote/webdriver.py", line 238, in execute
 self.error_handler.check_response(response)
   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
 ges/selenium/webdriver/remote/errorhandler.py", line 193, in
 check_response
 raise exception_class(message, screen, stacktrace)
 UnexpectedAlertPresentException: Alert Text: None
 Message: unexpected alert open: {Alert text : Are you sure you wish to
 close the pgAdmin 4 browser?}
   (Session info: chrome=58.0.3029.81)
   (Driver info: chromedriver=2.29.461585 
 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac
 OS X 10.12.3 x86_64)


 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

Hi

On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> The support to handle [null] and [default] values is added for following
> formatters:
>  - JsonFormatter
>  - NumbersFormatter
>  - CheckmarkFormatter
>  - TextFormatter
>
> Previously when a new row is added, it was not validating each and every
> cell for columns with attribute not_null = true.
> Introduced a new function validateRow(...) which is called on adding a
> new row, it validates each cell with column having
> attribute(not_null=true). the corresponding cell will highlighted and save
> button will be disabled if value is [null].
>

I'm not sure that behaviour is right. What I now see (given the table
below) is that:

- If I click in the id field of a new row, it forces me to enter a value or
hit escape. Why is it trying to force me? It's a serial field, so will get
a value on save anyway.

- If I do enter a value in the ID field, focus jumps over all the other
fields with either a default or no 'not null' option to the data_no_nulls
column. Focus should always go to the next field.

I think this addition can just be removed - I'm pretty sure the previous
behaviour would have been what we want, with the additional formatters
fixed.


>
> Now I will add more feature test cases for remaining formatters. Will send
> separate patch for feature test cases once completed.
>

Thanks.


>
> Please review updated patch.
>
>
> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> On Tue, May 2, 2017 at 5:21 PM, Dave Page  wrote:
>>
>>> Hi,
>>>
>>> This is looking much better now :-). Couple of thoughts and a bug:
>>>
>>> - Only the TextFormatter seems to handle both [null] and [default]
>>> values. Shouldn't all formatters do so (including Json and Checkmark)?
>>>
>> ​Yes, I will apply the same changes for other formatters too.​
>>
>>> For example, "serial" columns currently get displayed as [null] when
>>> left blank, but I would expect to see [default].
>>>
>>> - I would suggest we put [null] and [default] in a lighter colour -
>>> #99.
>>>
>>> - With the feature test patch added, I seem to be consistently getting
>>> the following failure (immediately after your new tests run):
>>>
>>> ==
>>> ERROR: runTest (pgadmin.feature_tests.xss_che
>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest)
>>> Test XSS check for panels and query tool
>>> --
>>> Traceback (most recent call last):
>>>   File 
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>>> line 42, in setUp
>>> self._screenshot()
>>>   File 
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>>> line 92, in _screenshot
>>> python_version))
>>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in
>>> get_screenshot_as_file
>>> png = self.get_screenshot_as_png()
>>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in
>>> get_screenshot_as_png
>>> return base64.b64decode(self.get_screenshot_as_base64().encode('asc
>>> ii'))
>>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in
>>> get_screenshot_as_base64
>>> return self.execute(Command.SCREENSHOT)['value']
>>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute
>>> self.error_handler.check_response(response)
>>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in
>>> check_response
>>> raise exception_class(message, screen, stacktrace)
>>> UnexpectedAlertPresentException: Alert Text: None
>>> Message: unexpected alert open: {Alert text : Are you sure you wish to
>>> close the pgAdmin 4 browser?}
>>>   (Session info: chrome=58.0.3029.81)
>>>   (Driver info: chromedriver=2.29.461585 
>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac
>>> OS X 10.12.3 x86_64)
>>>
>>>
>>> ==
>>> ERROR: runTest (pgadmin.feature_tests.xss_che
>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest)
>>> Test table DDL generation
>>> --
>>> Traceback (most recent call last):
>>>   File 
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>>> line 42, in setUp
>>> self._screenshot()
>>>   File 
>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>>> line 92, in _screenshot
>>> python_version))
>>> 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

The support to handle [null] and [default] values is added for following
formatters:
 - JsonFormatter
 - NumbersFormatter
 - CheckmarkFormatter
 - TextFormatter

Previously when a new row is added, it was not validating each and every
cell for columns with attribute not_null = true.
Introduced a new function validateRow(...) which is called on adding a new
row, it validates each cell with column having attribute(not_null=true).
the corresponding cell will highlighted and save button will be disabled if
value is [null].

Now I will add more feature test cases for remaining formatters. Will send
separate patch for feature test cases once completed.

Please review updated patch.


On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> On Tue, May 2, 2017 at 5:21 PM, Dave Page  wrote:
>
>> Hi,
>>
>> This is looking much better now :-). Couple of thoughts and a bug:
>>
>> - Only the TextFormatter seems to handle both [null] and [default]
>> values. Shouldn't all formatters do so (including Json and Checkmark)?
>>
> ​Yes, I will apply the same changes for other formatters too.​
>
>> For example, "serial" columns currently get displayed as [null] when left
>> blank, but I would expect to see [default].
>>
>> - I would suggest we put [null] and [default] in a lighter colour -
>> #99.
>>
>> - With the feature test patch added, I seem to be consistently getting
>> the following failure (immediately after your new tests run):
>>
>> ==
>> ERROR: runTest (pgadmin.feature_tests.xss_che
>> cks_panels_and_query_tool_test.CheckForXssFeatureTest)
>> Test XSS check for panels and query tool
>> --
>> Traceback (most recent call last):
>>   File 
>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>> line 42, in setUp
>> self._screenshot()
>>   File 
>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>> line 92, in _screenshot
>> python_version))
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 802, in
>> get_screenshot_as_file
>> png = self.get_screenshot_as_png()
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 821, in
>> get_screenshot_as_png
>> return base64.b64decode(self.get_screenshot_as_base64().encode('
>> ascii'))
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 831, in
>> get_screenshot_as_base64
>> return self.execute(Command.SCREENSHOT)['value']
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute
>> self.error_handler.check_response(response)
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in
>> check_response
>> raise exception_class(message, screen, stacktrace)
>> UnexpectedAlertPresentException: Alert Text: None
>> Message: unexpected alert open: {Alert text : Are you sure you wish to
>> close the pgAdmin 4 browser?}
>>   (Session info: chrome=58.0.3029.81)
>>   (Driver info: chromedriver=2.29.461585 
>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac
>> OS X 10.12.3 x86_64)
>>
>>
>> ==
>> ERROR: runTest (pgadmin.feature_tests.xss_che
>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest)
>> Test table DDL generation
>> --
>> Traceback (most recent call last):
>>   File 
>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>> line 42, in setUp
>> self._screenshot()
>>   File 
>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
>> line 92, in _screenshot
>> python_version))
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 802, in
>> get_screenshot_as_file
>> png = self.get_screenshot_as_png()
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 821, in
>> get_screenshot_as_png
>> return base64.b64decode(self.get_screenshot_as_base64().encode('
>> ascii'))
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 831, in
>> get_screenshot_as_base64
>> return self.execute(Command.SCREENSHOT)['value']
>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute
>> self.error_handler.check_response(response)

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

On Tue, May 2, 2017 at 5:21 PM, Dave Page  wrote:

> Hi,
>
> This is looking much better now :-). Couple of thoughts and a bug:
>
> - Only the TextFormatter seems to handle both [null] and [default] values.
> Shouldn't all formatters do so (including Json and Checkmark)?
>
​Yes, I will apply the same changes for other formatters too.​

> For example, "serial" columns currently get displayed as [null] when left
> blank, but I would expect to see [default].
>
> - I would suggest we put [null] and [default] in a lighter colour -
> #99.
>
> - With the feature test patch added, I seem to be consistently getting the
> following failure (immediately after your new tests run):
>
> ==
> ERROR: runTest (pgadmin.feature_tests.xss_checks_panels_and_query_tool_
> test.CheckForXssFeatureTest)
> Test XSS check for panels and query tool
> --
> Traceback (most recent call last):
>   File 
> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
> line 42, in setUp
> self._screenshot()
>   File 
> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
> line 92, in _screenshot
> python_version))
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/webdriver.py", line 802, in
> get_screenshot_as_file
> png = self.get_screenshot_as_png()
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/webdriver.py", line 821, in
> get_screenshot_as_png
> return base64.b64decode(self.get_screenshot_as_base64().encode(
> 'ascii'))
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/webdriver.py", line 831, in
> get_screenshot_as_base64
> return self.execute(Command.SCREENSHOT)['value']
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/webdriver.py", line 238, in execute
> self.error_handler.check_response(response)
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/errorhandler.py", line 193, in
> check_response
> raise exception_class(message, screen, stacktrace)
> UnexpectedAlertPresentException: Alert Text: None
> Message: unexpected alert open: {Alert text : Are you sure you wish to
> close the pgAdmin 4 browser?}
>   (Session info: chrome=58.0.3029.81)
>   (Driver info: chromedriver=2.29.461585 
> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac
> OS X 10.12.3 x86_64)
>
>
> ==
> ERROR: runTest (pgadmin.feature_tests.xss_checks_pgadmin_debugger_test.
> CheckDebuggerForXssFeatureTest)
> Test table DDL generation
> --
> Traceback (most recent call last):
>   File 
> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
> line 42, in setUp
> self._screenshot()
>   File 
> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
> line 92, in _screenshot
> python_version))
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/webdriver.py", line 802, in
> get_screenshot_as_file
> png = self.get_screenshot_as_png()
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/webdriver.py", line 821, in
> get_screenshot_as_png
> return base64.b64decode(self.get_screenshot_as_base64().encode(
> 'ascii'))
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/webdriver.py", line 831, in
> get_screenshot_as_base64
> return self.execute(Command.SCREENSHOT)['value']
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/webdriver.py", line 238, in execute
> self.error_handler.check_response(response)
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/selenium/webdriver/remote/errorhandler.py", line 193, in
> check_response
> raise exception_class(message, screen, stacktrace)
> UnexpectedAlertPresentException: Alert Text: None
> Message: unexpected alert open: {Alert text : Are you sure you wish to
> close the pgAdmin 4 browser?}
>   (Session info: chrome=58.0.3029.81)
>   (Driver info: chromedriver=2.29.461585 
> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac
> OS X 10.12.3 x86_64)
>
​Sure. I will fix this.​

>
> Thanks!
>
>
> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar <
> surinder.ku...@enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> Please find updated patch for RM case and a separate patch for Feature
>> tests.
>>
>> *Python:*
>>
>> - Added [default] label for cells with default values while inserting a
>> new row.
>>
>> - 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

Hi,

This is looking much better now :-). Couple of thoughts and a bug:

- Only the TextFormatter seems to handle both [null] and [default] values.
Shouldn't all formatters do so (including Json and Checkmark)? For example,
"serial" columns currently get displayed as [null] when left blank, but I
would expect to see [default].

- I would suggest we put [null] and [default] in a lighter colour - #99.

- With the feature test patch added, I seem to be consistently getting the
following failure (immediately after your new tests run):

==
ERROR: runTest
(pgadmin.feature_tests.xss_checks_panels_and_query_tool_test.CheckForXssFeatureTest)
Test XSS check for panels and query tool
--
Traceback (most recent call last):
  File
"/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
line 42, in setUp
self._screenshot()
  File
"/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
line 92, in _screenshot
python_version))
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
line 802, in get_screenshot_as_file
png = self.get_screenshot_as_png()
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
line 821, in get_screenshot_as_png
return base64.b64decode(self.get_screenshot_as_base64().encode('ascii'))
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
line 831, in get_screenshot_as_base64
return self.execute(Command.SCREENSHOT)['value']
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
line 238, in execute
self.error_handler.check_response(response)
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py",
line 193, in check_response
raise exception_class(message, screen, stacktrace)
UnexpectedAlertPresentException: Alert Text: None
Message: unexpected alert open: {Alert text : Are you sure you wish to
close the pgAdmin 4 browser?}
  (Session info: chrome=58.0.3029.81)
  (Driver info: chromedriver=2.29.461585
(0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X 10.12.3 x86_64)


==
ERROR: runTest
(pgadmin.feature_tests.xss_checks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest)
Test table DDL generation
--
Traceback (most recent call last):
  File
"/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
line 42, in setUp
self._screenshot()
  File
"/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py",
line 92, in _screenshot
python_version))
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
line 802, in get_screenshot_as_file
png = self.get_screenshot_as_png()
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
line 821, in get_screenshot_as_png
return base64.b64decode(self.get_screenshot_as_base64().encode('ascii'))
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
line 831, in get_screenshot_as_base64
return self.execute(Command.SCREENSHOT)['value']
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
line 238, in execute
self.error_handler.check_response(response)
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py",
line 193, in check_response
raise exception_class(message, screen, stacktrace)
UnexpectedAlertPresentException: Alert Text: None
Message: unexpected alert open: {Alert text : Are you sure you wish to
close the pgAdmin 4 browser?}
  (Session info: chrome=58.0.3029.81)
  (Driver info: chromedriver=2.29.461585
(0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X 10.12.3 x86_64)

Thanks!


On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi Dave,
>
> Please find updated patch for RM case and a separate patch for Feature
> tests.
>
> *Python:*
>
> - Added [default] label for cells with default values while inserting a
> new row.
>
> - Introduced a FieldValidator function for cells that don't allow null
> values. If user tries to insert null value, field with be highlighted with
> redborders around.
>
> ​-
> If a cell contains blank string('') and when we set it to null, the change
> into the cell is not detected. It was because the comparison
> for (defaultValue == null) return true if defaultValue is undefined. Hence
> 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi Dave,

Please find updated patch for RM case and a separate patch for Feature
tests.

*Python:*

- Added [default] label for cells with default values while inserting a new
row.

- Introduced a FieldValidator function for cells that don't allow null
values. If user tries to insert null value, field with be highlighted with
redborders around.

​-
If a cell contains blank string('') and when we set it to null, the change
into the cell is not detected. It was because the comparison
for (defaultValue == null) return true if defaultValue is undefined. Hence
_.isNull(value) is used to fix this.

*Feature Test cases:*

 - Introduced a new method create_table_with_query(server, db_name, query)
in test_utils.py which executes the given query on connected server.

 - Added a new file test_data.json that has test data for test cases.


On Fri, Apr 7, 2017 at 2:21 PM, Dave Page  wrote:

> Hi
>
> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar
>  wrote:
> > Hi
> >
> > Issues fixed:
> >
> > 1. If a column is defined with a default modifier, there is now way to
> > insert the row with those defaults.
> > The column will be left blank and it will take default value
> automatically.
> >
> > 2. If a column has a not-null constraint then an error is returned and
> the
> > row is not inserted.
> > The column will be left blank
> >
> > The default values for new added rows will be displayed on
> refresh/execute.
> >
> > Please find attached patch and review.
>
> This largely works as expected, but there is some weirdness. I have a
> test table that looks like this:
>
> CREATE TABLE public.defaults
> (
> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass),
> data_default_nulls text COLLATE pg_catalog."default" DEFAULT
> 'abc123'::text,
> data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL
> DEFAULT 'def456'::text,
> data_nulls text COLLATE pg_catalog."default",
> data_no_nulls text COLLATE pg_catalog."default" NOT NULL,
> CONSTRAINT defaults_pkey PRIMARY KEY (id)
> )
>
> Remember that the expected behaviour is:
>
> - Set a value to empty to update the column to null.
> - Set a value to '' to update the column to an empty string
> - Set a value to anything else to update the column to that value
>
> 1) In a row with values in each column, if I try to set the value of
> data_default_nulls to null, the query executed is:
>
> UPDATE public.defaults SET
> data_default_nulls = '' WHERE
> id = '2';
>
> 2) If I do the same in the data_nulls column, the value is immediately
> shown as [null] and the query executed is:
>
> UPDATE public.defaults SET
> data_nulls = NULL WHERE
> id = '2';
>
> 3) If I then edit the value in data_default_nulls, it shows the
> current value as ''. Removing the quotes (to set it to null) doesn't
> get detected as a change.
>
​​Taken care.

>
> 4) When I manually executed "update defaults set data_default_nulls =
> null where id = 2" in a query tool window, I got:
>
> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017
> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 -
> Traceback (most recent call last):
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask/app.py",
> line 2000, in __call__
> return self.wsgi_app(environ, start_response)
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask/app.py",
> line 1991, in wsgi_app
> response = self.make_response(self.handle_exception(e))
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask/app.py",
> line 1567, in handle_exception
> reraise(exc_type, exc_value, tb)
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask/app.py",
> line 1988, in wsgi_app
> response = self.full_dispatch_request()
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask/app.py",
> line 1641, in full_dispatch_request
> rv = self.handle_user_exception(e)
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask/app.py",
> line 1544, in handle_user_exception
> reraise(exc_type, exc_value, tb)
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask/app.py",
> line 1639, in full_dispatch_request
> rv = self.dispatch_request()
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask/app.py",
> line 1625, in dispatch_request
> return self.view_functions[rule.endpoint](**req.view_args)
>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
> packages/flask_login.py",
> line 792, in decorated_view
> return func(*args, **kwargs)
>   File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__
> init__.py",
> line 452, in get_columns
> tid=command_obj.obj_id)
> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id'
>
​Fixed.​

>
> 5) When I run the query again in pgAdmin III, then refresh the 

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

On Tue, Apr 4, 2017 at 5:37 PM, Matthew Kleiman  wrote:
> Hi Surinder,
>
> We looked at your fix for default values in the query tool editor.
>
> We think the user experience could be further improved by seeing the actual
> default value instead of the empty cell.

Agreed.

> As an intermediate step, there would be user value in seeing "[default]"
> instead of the blank cell (or what used to be "[null]").

Also agreed. Surinder, please add the [default] label in your next
update. We'll look at immediately returning values in the future,
though that may not actually be possible without saving the row
(defaults may not be constant, they could be expressions).


-- 
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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Dave Page]

Hi

On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar
 wrote:
> Hi
>
> Issues fixed:
>
> 1. If a column is defined with a default modifier, there is now way to
> insert the row with those defaults.
> The column will be left blank and it will take default value automatically.
>
> 2. If a column has a not-null constraint then an error is returned and the
> row is not inserted.
> The column will be left blank
>
> The default values for new added rows will be displayed on refresh/execute.
>
> Please find attached patch and review.

This largely works as expected, but there is some weirdness. I have a
test table that looks like this:

CREATE TABLE public.defaults
(
id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass),
data_default_nulls text COLLATE pg_catalog."default" DEFAULT 'abc123'::text,
data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL
DEFAULT 'def456'::text,
data_nulls text COLLATE pg_catalog."default",
data_no_nulls text COLLATE pg_catalog."default" NOT NULL,
CONSTRAINT defaults_pkey PRIMARY KEY (id)
)

Remember that the expected behaviour is:

- Set a value to empty to update the column to null.
- Set a value to '' to update the column to an empty string
- Set a value to anything else to update the column to that value

1) In a row with values in each column, if I try to set the value of
data_default_nulls to null, the query executed is:

UPDATE public.defaults SET
data_default_nulls = '' WHERE
id = '2';

2) If I do the same in the data_nulls column, the value is immediately
shown as [null] and the query executed is:

UPDATE public.defaults SET
data_nulls = NULL WHERE
id = '2';

3) If I then edit the value in data_default_nulls, it shows the
current value as ''. Removing the quotes (to set it to null) doesn't
get detected as a change.

4) When I manually executed "update defaults set data_default_nulls =
null where id = 2" in a query tool window, I got:

2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017
09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 -
Traceback (most recent call last):
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 2000, in __call__
return self.wsgi_app(environ, start_response)
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1991, in wsgi_app
response = self.make_response(self.handle_exception(e))
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1567, in handle_exception
reraise(exc_type, exc_value, tb)
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1988, in wsgi_app
response = self.full_dispatch_request()
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1641, in full_dispatch_request
rv = self.handle_user_exception(e)
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1544, in handle_user_exception
reraise(exc_type, exc_value, tb)
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1639, in full_dispatch_request
rv = self.dispatch_request()
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1625, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
  File 
"/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask_login.py",
line 792, in decorated_view
return func(*args, **kwargs)
  File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__init__.py",
line 452, in get_columns
tid=command_obj.obj_id)
AttributeError: 'QueryToolCommand' object has no attribute 'obj_id'

5) When I run the query again in pgAdmin III, then refresh the data in
pgAdmin 4, the data_default_nulls column is displayed without the
[null] marker (despite having a null value, which I confirmed in
pgAdmin 3).

I'm sure there are other combinations of issues here. Please fix and
thoroughly re-test to ensure behaviour is consistent - and to avoid
future issues, please add some appropriate feature tests to check
nulls, defaults and empty strings are properly handled in view, insert
and updates. Murtuza recently wrote some feature tests for the query
tool - you should be able to use those as a starting point.

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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Re: [pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Matthew Kleiman]

Hi Surinder,

We looked at your fix for default values in the query tool editor.

We think the user experience could be further improved by seeing the actual
default value instead of the empty cell.

As an intermediate step, there would be user value in seeing "[default]"
instead of the blank cell (or what used to be "[null]").

Thanks,
George and Matt



On Sat, Apr 1, 2017 at 7:45 AM, Surinder Kumar <
surinder.ku...@enterprisedb.com> wrote:

> Hi
>
> Issues fixed:
>
> 1. If a column is defined with a default modifier, there is now way to
> insert the row with those defaults.
> The column will be left blank and it will take default value automatically.
>
> 2. If a column has a not-null constraint then an error is returned and
> the row is not inserted.
> The column will be left blank
> ​ ​
>
> ​The default values for new added rows will be displayed on
> refresh/execute.
>
> Please find attached patch and review.
>
>
> Thanks
> Surinder
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>
>

[pgadmin-hackers] [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

[Surinder Kumar]

Hi

Issues fixed:

1. If a column is defined with a default modifier, there is now way to
insert the row with those defaults.
The column will be left blank and it will take default value automatically.

2. If a column has a not-null constraint then an error is returned and the
row is not inserted.
The column will be left blank
​ ​

​The default values for new added rows will be displayed on refresh/execute.

Please find attached patch and review.


Thanks
Surinder


RM_2257.patch
Description: Binary data

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