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

Hi Dave,

On Mon, May 8, 2017 at 4:37 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

>
>
> On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar <
> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>
>> Hi
>>
>> On Mon, May 8, 2017 at 3:51 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar <
>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave,
>>>>
>>>> On Mon, May 8, 2017 at 3:28 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
>>>>> surinder(dot)kumar(at)enterprisedb(dot)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(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Dave,
>>>>>>>
>>>>>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <dpage(at)pgadmin(dot)org> 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 -
>>>>>>>> #999999.
>>>>>>>>
>>>>>>>> - 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 "/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_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 "/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)
>>>>>>>>
>>>>>>> ​Sure. I will fix this.​
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>>
>>>>>>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar <
>>>>>>>> surinder(dot)kumar(at)enterprisedb(dot)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 red borders 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 <dpage(at)pgadmin(dot)org>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi
>>>>>>>>>>
>>>>>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar
>>>>>>>>>> <surinder(dot)kumar(at)enterprisedb(dot)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.
>>>>>>>>>>
>>>>>>>>>> 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'::reg
>>>>>>>>>> class),
>>>>>>>>>> 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/pga
>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py",
>>>>>>>>>> line 2000, in __call__
>>>>>>>>>> return self.wsgi_app(environ, start_response)
>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga
>>>>>>>>>> dmin4/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/pga
>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py",
>>>>>>>>>> line 1567, in handle_exception
>>>>>>>>>> reraise(exc_type, exc_value, tb)
>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga
>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py",
>>>>>>>>>> line 1988, in wsgi_app
>>>>>>>>>> response = self.full_dispatch_request()
>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga
>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py",
>>>>>>>>>> line 1641, in full_dispatch_request
>>>>>>>>>> rv = self.handle_user_exception(e)
>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga
>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py",
>>>>>>>>>> line 1544, in handle_user_exception
>>>>>>>>>> reraise(exc_type, exc_value, tb)
>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga
>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py",
>>>>>>>>>> line 1639, in full_dispatch_request
>>>>>>>>>> rv = self.dispatch_request()
>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga
>>>>>>>>>> dmin4/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/pga
>>>>>>>>>> dmin4/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
>>>>>>>>>> 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).
>>>>>>>>>>
>>>>>>>>> ​Fixed.​
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>> ​Added feature tests​
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> 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
>