Re: [pgAdmin4][Patch][RM_2400]: Columns with defaults set to NULL when removing contents after pasting in the edit grid

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

​

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?

Does _check_xss_in_view_data method checks for Cross Site Scripting?

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

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

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_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
>
> Also, Can we replace the sleep with a "wait for object" or similar?
>
> 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)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
>