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 <dp...@pgadmin.org> 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 <dp...@pgadmin.org> 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 <dp...@pgadmin.org> 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 <dp...@pgadmin.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/pga
>>>>>>>>> dmin4/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/pga
>>>>>>>>> dmin4/lib/python2.7/site-packages/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/pga
>>>>>>>>> dmin4/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/pga
>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
>>>>>>>>> line 238, in execute
>>>>>>>>> self.error_handler.check_response(response)
>>>>>>>>> File "/Users/dpage/.virtualenvs/pga
>>>>>>>>> dmin4/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_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/pga
>>>>>>>>> dmin4/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/pga
>>>>>>>>> dmin4/lib/python2.7/site-packages/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/pga
>>>>>>>>> dmin4/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/pga
>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py",
>>>>>>>>> line 238, in execute
>>>>>>>>> self.error_handler.check_response(response)
>>>>>>>>> File "/Users/dpage/.virtualenvs/pga
>>>>>>>>> dmin4/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.
>>>>>>>>>>
>>>>>>>>>> - 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 <dp...@pgadmin.org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi
>>>>>>>>>>>
>>>>>>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, 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.
>>>>>>>>>>>
>>>>>>>>>>> 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
>>
>
>
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
