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

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(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_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)
>> 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'::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-packa
>>>> ges/flask/app.py",
>>>> line 2000, in __call__
>>>> return self.wsgi_app(environ, start_response)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>>> ges/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-packa
>>>> ges/flask/app.py",
>>>> line 1567, in handle_exception
>>>> reraise(exc_type, exc_value, tb)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>>> ges/flask/app.py",
>>>> line 1988, in wsgi_app
>>>> response = self.full_dispatch_request()
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>>> ges/flask/app.py",
>>>> line 1641, in full_dispatch_request
>>>> rv = self.handle_user_exception(e)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>>> ges/flask/app.py",
>>>> line 1544, in handle_user_exception
>>>> reraise(exc_type, exc_value, tb)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>>> ges/flask/app.py",
>>>> line 1639, in full_dispatch_request
>>>> rv = self.dispatch_request()
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>>> ges/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-packa
>>>> ges/flask_login.py",
>>>> line 792, in decorated_view
>>>> return func(*args, **kwargs)
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ini
>>>> t__.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
>>
>
>