Re: [pgadmin-hackers] [pgAdmin4][Patch][RM_2400]: Columns withdefaults set to NULL when removing contents after pasting in the edit grid

Hi

Please find updated patch.

On Fri, May 26, 2017 at 3:15 AM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:

Hello Surinder,

We are having issues running the tests, this is the error that we are getting:

  File "/Users/pivotal/workspace/pgadmin4/web/regression/python_test_utils/test_utils.py", line 196, in create_table_with_query   pg_cursor.execute(query)
ProgrammingError: role "postgres" does not exist

Traceback (most recent call last): File "/Users/pivotal/workspace/pgadmin4/web/regression/python_test_utils/test_utils.py", line 196, in create_table_with_query   pg_cursor.execute(query)
ProgrammingError: relation "defaults_id_seq" does not exist

​

​Fixed.​

There is already a function that waits for an element to be displayed on the screen. The function is: self.page.find_by_xpath

In line 179 and 180, both functions do the same thing, why do we need to wait first and then wait again. Were you experiencing flakiness?

I have to use another instance of webDriverWait because I was getting TimeoutException. I guess timeout of 10 seconds wasn't enough to search the element in DOM.

Does _check_xss_in_view_data method checks for Cross Site Scripting?

​I forgot to change the method name.​

Was there any reason to duplicate self.page._connects_to_server and self.page._close_query_tool?

​Fixed.

I got following exception when I used ​"self.page._close_query_tool":

Traceback (most recent call last):
File "/Users/surinder/Documents/Projects/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py", line 125, in runTest
self.page.close_query_tool()
File "/Users/surinder/Documents/Projects/pgadmin4/web/regression/feature_utils/pgadmin_page.py", line 66, in close_query_tool
self.driver.switch_to.frame(self.driver.find_elements_by_tag_name("iframe")[0])
File "/Users/surinder/Documents/Workspaces/PEM_35/lib/python3.5/site-packages/selenium/webdriver/remote/switch_to.py", line 87, in frame
self._driver.execute(Command.SWITCH_TO_FRAME, {'id': frame_reference})
File "/Users/surinder/Documents/Workspaces/PEM_35/lib/python3.5/site-packages/selenium/webdriver/remote/webdriver.py", line 238, in execute
self.error_handler.check_response(response)
File "/Users/surinder/Documents/Workspaces/PEM_35/lib/python3.5/site-packages/selenium/webdriver/remote/errorhandler.py", line 193, in check_response
raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.WebDriverException: Message: unknown error: Runtime.evaluate threw exception: Error: element is not attached to the page document
at Cache.retrieveItem (<anonymous>:173:17)
at unwrap (<anonymous>:293:20)
at unwrap (<anonymous>:297:19)
at callFunction (<anonymous>:343:29)
at apply.ELEMENT (<anonymous>:357:23)
at <anonymous>:358:3
(Session info: chrome=58.0.3029.110)
(Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X 10.10.2 x86_64)

I replaced this method with the one I was using in the test file and It is working for every test case. 

The method _verify_insert_data looks more or less the same code as in the end of _copy_paste_row, should this be merged?

​Yes, I have merged.

Thanks​

Thanks,

Joao & Shruti

On Thu, May 25, 2017 at 4:41 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

The tests failed on both PG 9.4 and 9.6 for me :-(

======================================================================
ERROR: runTest (pgadmin.feature_tests.view_data_dml_queries.CheckForViewDataTest)
Validate Insert, Update operations in View data with given test data
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
line 120, in runTest
    self._verify_insert_data()
  File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
line 316, in _verify_insert_data
    self._compare_cell_value(cell_xpath, config_data[str(idx)][1])
  File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_data_dml_queries.py",
line 165, in _compare_cell_value
    "Timed out waiting for element to appear"
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/support/wait.py",
line 80, in until
    raise TimeoutException(message, screen, stacktrace)
TimeoutException: Message: Timed out waiting for element to appear

​​I increases the timeout of webDriverWait from "0.3" to "0.8". It should fix this issue.

Also, Can we replace the sleep with a "wait for object" or similar?

​I have removed sleep.​

On Thu, May 25, 2017 at 6:42 AM, Surinder Kumar

<surinder.kumar@enterprisedb.com> wrote:
> Hi
>
> Please find attached patch for Feature test cases.
>
> The approach is to create a single table 'defaults' with columns of various
> types(number, text, json and boolean) and write test cases for these cell
> types with different input values.
>
> Following are the test cases covered:
>
> 1) Add a new row, save and compare the resulted value with expected values
>
> 2) Copy/Paste row, save and compare cell data
>
>     a) Clear cell value and escape, the cell must set to [default]
>
> 3) Update cell:
>
>    a) Insert two single quotes(''), expected value is blank string
>
>    b) Clear a cell, expected value is [null]
>
>    c) Insert a string \’\’, expected value is ''
>
>    d) Insert a string \\’\\’, expected value is \’\’
>
>    e) Insert a string \\\\’\\\\’, expected value is \\’\\’
>
>    f)  If a checkbox cell is double clicked, return value must be 'true'
>
>
> Thanks
> Surinder Kumar
>
>
>
>
>
>
> On Fri, May 19, 2017 at 6:08 PM, Surinder Kumar
> <surinder.kumar@enterprisedb.com> wrote:
>>
>> Hi
>>
>> Please find updated patch and review.
>>
>> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> <jdealmeidapereira@pivotal.io> wrote:
>>>
>>> Hello Hackers,
>>>
>>> We reviewed the PR, and there are a lot of new lines of code in this
>>> patch, are we sure that we can have a good coverage of the functionality
>>> just by doing feature tests?
>>> Adding more code to a 3.5k+ lines file doesn't look like a good option,
>>> do you think it is possible to extract some of the functionality to their
>>> own files and have test around those functionalities?
>>
>> To improve the code readability, reduce code complexity and to make code
>> testable, the code must be splitted component wise.
>> Here is my suggestion:
>>
>> 1. The code for classes like “SQLEditorView” and “SqlEditorController” can
>> be moved into two files like "editor_view.js and “editor_controller.js" and
>> called from within "sqleditor.js".
>>
>> 2. All utilities functions can be moved into separate utils file and can
>> write test cases.
>>
>> 3. Slickgrid listener functions such as:
>> onBeforeEditCell, onKeyDown,
>> onCellChange, onAddNewRow
>>
>> can be moved into
>> a file and write test cases around.
>>
>> This needs discussion. Any suggestion?
>>>
>>>
>>> Do we really need to have an epicRandomString function in our code? Would
>>> it be better to use a library that would provide us that functionality out
>>> of the box?
>>
>> We are using "epicRandomString function" to uniquely identify each record,
>> I talked to Harshal who is eliminating the use of this function and instead
>> maintaining counter for the rows added/updated/deleted.
>>>
>>> The functions this.applyValue in slick.pgadmin.editors.js that were
>>> change in this patch are exactly the same, can we extract that code into a
>>> single function instead of repeating the code?
>>
>> I have moved common logic into a new function.
>>>
>>>
>>> Using feature tests is a good option to ensure that the integration of
>>> all the components of the application is working as expected, but in order
>>> to tests behaviors that are edge cases the Unit Tests are much cheaper to
>>> run and create.
>>
>> I will write test cases for functions once they are moved into their
>> separate files as discussed above.
>>>
>>>
>>> Thanks
>>> Joao & Shruti
>>>
>>>
>>> On Thu, May 18, 2017 at 1:22 AM, Surinder Kumar
>>> <surinder.kumar@enterprisedb.com> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> Please review the updated patch.
>>>>
>>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>>>>> <surinder.kumar@enterprisedb.com> wrote:
>>>>>>
>>>>>> Hi Dave,
>>>>>>
>>>>>> Implementation:
>>>>>>
>>>>>> 1) Took a flag 'is_row_copied' for each copied row:
>>>>>>
>>>>>>  - to distinguish it from add/update row.
>>>>>>  - to write code specific to copied row only as it requires different
>>>>>> logic than add row.
>>>>>>
>>>>>> 2) After pasting an existing row:
>>>>>>
>>>>>>  - If a user clear the cell value, it must set cell to [default] value
>>>>>> if default value is explicitly given for column while creating table
>>>>>> otherwise [null].
>>>>>>
>>>>>>  - Again, if a user entered a value in same cell, then removes that
>>>>>> value, set it to [null] (the same behaviour is for add row).
>>>>>>
>>>>>> 3) Took a 2-dimensional array "grid.copied_rows" to keep track the
>>>>>> changes of each row's cell so that changes made to each cell are independent
>>>>>> and removed once data is saved.
>>>>>>
>>>>>> 4) On pasting a row, the cell must be highlighted with light green
>>>>>> colour, so triggers an addNewRow event. Now copied row will add to grid
>>>>>> instead of re-rendering all rows again.
>>>>>>
>>>>>> 5) Moved the common logic into functions.
>>>>>>
>>>>>> This patch pass the feature test cases and jasmine test case.
>>>>>> Also I will add the test cases for copy row and send an updated patch.
>>>>>>
>>>>>> Please find attached patch and review.
>>>>>
>>>>>
>>>>> Looks good. I saw the following issues:
>>>>>
>>>>> - The "new" row is not available once I've created the first new row,
>>>>> or pasted some.
>>>>
>>>> Fixed.
>>>>>
>>>>>
>>>>> - Rows are pasted in reverse order.
>>>>
>>>> Fixed.
>>>>>
>>>>>
>>>>> - If I copy/paste a new row that has yet to be saved, none of the
>>>>> values are actually copied.
>>>>
>>>> Fixed.
>>>>>
>>>>>
>>>>> - Attempting to save a row that contains all null/default values
>>>>> results in an invalid query:
>>>>>
>>>>> INSERT INTO public.defaults (
>>>>> ) VALUES (
>>>>> );
>>>>>
>>>>> I think the only answer here is to explicitly insert NULL into any
>>>>> columns *without* a default value.
>>>>
>>>> I could not reproduce, However  #3 might have fixed it too.
>>>>
>>>> Apart from this, I noticed epicRandomString(...) function doesn't
>>>> generate unique number always, due to this save and delete rows was
>>>> affected. Not all rows saved or deleted. The new function always returns a
>>>> unique random number.
>>>> Fixed.
>>>>>
>>>>>
>>>>> 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
>>>>
>>>
>>
>



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

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