Re: [pgAdmin4][Patch][RM_2400]: Columns with defaults 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(at)pivotal(dot)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/fea
> ture_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(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> The tests failed on both PG 9.4 and 9.6 for me :-(
>>
>> ======================================================================
>> ERROR: runTest (pgadmin.feature_tests.view_da
>> ta_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_da
>> ta_dml_queries.py",
>> line 120, in runTest
>> self._verify_insert_data()
>> File "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/view_da
>> ta_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_da
>> ta_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-packa
>> ges/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(dot)kumar(at)enterprisedb(dot)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(dot)kumar(at)enterprisedb(dot)com> wrote:
>> >>
>> >> Hi
>> >>
>> >> Please find updated patch and review.
>> >>
>> >> On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira
>> >> <jdealmeidapereira(at)pivotal(dot)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(dot)kumar(at)enterprisedb(dot)com> wrote:
>> >>>>
>> >>>> Hi Dave,
>> >>>>
>> >>>> Please review the updated patch.
>> >>>>
>> >>>> On Wed, May 17, 2017 at 8:46 PM, Dave Page <dpage(at)pgadmin(dot)org>
>> wrote:
>> >>>>>
>> >>>>> Hi
>> >>>>>
>> >>>>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar
>> >>>>> <surinder(dot)kumar(at)enterprisedb(dot)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(at)postgresql(dot)or
>> g)
>> >>>> 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
>>
>
>