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

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 :-)

>
>>
>
>>>>
>>>>>
>>>>> 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