Re: [pgadmin-hackers] [pgAdmin4] [PATCH] History Tab rewrite in React

Hi Shruti

On Mon, Jun 12, 2017 at 3:24 PM, Shruti B Iyer <siyer@pivotal.io> wrote:
>
> Hello Dave,
>
> Thanks for making those fixes and sharing them with us. We tried applying
> the patch and it looks like there are some missing file changes from your
> patch that were present in ours, like the Make.bat file changes. But we will
> add them when we send you the new patches.

Hmm, I wonder if I missed them because I applied the patch in a sub directory.

> While trying to generate the new patches we realized some tests are failing
> in master branch due to an internal server error:
>
> 2017-06-12 10:04:11,938: INFO werkzeug: 127.0.0.1 - - [12/Jun/2017 10:04:11]
> "GET /browser/table/sql/1/1/12669/2200/81920 HTTP/1.1" 500 -
> Traceback (most recent call last):
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py",
> line 2000, in __call__
>     return self.wsgi_app(environ, start_response)
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py",
> line 1991, in wsgi_app
>     response = self.make_response(self.handle_exception(e))
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py",
> line 1567, in handle_exception
>     reraise(exc_type, exc_value, tb)
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py",
> line 1988, in wsgi_app
>     response = self.full_dispatch_request()
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py",
> line 1641, in full_dispatch_request
>     rv = self.handle_user_exception(e)
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py",
> line 1544, in handle_user_exception
>     reraise(exc_type, exc_value, tb)
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py",
> line 1639, in full_dispatch_request
>     rv = self.dispatch_request()
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py",
> line 1625, in dispatch_request
>     return self.view_functions[rule.endpoint](**req.view_args)
>   File
> "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/views.py",
> line 84, in view
>     return self.dispatch_request(*args, **kwargs)
>   File "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/utils.py",
> line 235, in dispatch_request
>     return method(*args, **kwargs)
>   File
> "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py",
> line 315, in wrap
>     return f(*args, **kwargs)
>   File
> "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py",
> line 2555, in sql
>     data = self._formatter(did, scid, tid, data)
>   File
> "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py",
> line 1081, in _formatter
>     data = self._columns_formatter(tid, data)
>   File
> "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py",
> line 663, in _columns_formatter
>     column['attlen'] = matchObj.group(1)
> AttributeError: 'NoneType' object has no attribute 'group'
>
> Was this introduced in a previous patch?

Yes, it looks like it. For some reason it's not failing on the Jenkins
server though. I'll ask Khushboo to fix it.

> We will recreate the patches and send them ASAP.

Thanks!

> Thanks
> Shruti & Joao
>
> On Mon, Jun 12, 2017 at 6:59 AM Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> OK, so Ashesh and I spend much of the morning on this.
>>
>> Patch 01 - Applied.
>> Patch 02:
>>
>> - karma.conf.js wouldn't patch; I've manually handled that.
>> - test-main.js wouldn't patch. The diff looked like it was trying to
>> empty it; I have removed it instead.
>> - The imports in pgAdmin4.py need to be made after the app root is
>> added to the path.
>> - The JS bundler should be in pgadmin/utils, not pgadmin/tools (which
>> is intended for plugin modules)
>> - The tests were failing following some changes Ashesh pushed earlier
>> to add a client-side url_for function.
>> - pgAdmin4.py was attempting to run the bundler on every startup. I've
>> wrapped those called in "if config.DEBUG:" conditionals, as typically
>> an installation for an end-user will be in a read-only directory.
>>
>> We've fixed all of that in the attached patch. I'm not sure why it's
>> so much bigger than yours.
>>
>> The following issues are outstanding; please take a look at them:
>>
>> - There is no update to the Windows installer generation code (needed
>> in 2 places unfortunately; Make.bat and Make-MinGW.bat).
>>
>> - The updates to the other packages call "yarn run webpacker" which is
>> an undefined target.
>>
>> I haven't looked at patch 03 yet, but Ashesh did tell me it won't
>> apply for him. Patch 4 is also untested at this stage.
>>
>> If the issues above can be fixed, we can get patch 2 applied then move
>> on from there.
>>
>> I'll hold off on Harshal's patch for the Query Tool's load on demand
>> to give you a chance to get this done.
>>
>> Thanks.
>>
>> On Sat, Jun 10, 2017 at 2:52 AM, George Gelashvili
>> <ggelashvili@pivotal.io> wrote:
>> > Hi Dave,
>> >
>> > Our patch touches code also changed by the patches that were recently
>> > committed.
>> > That's likely what's causing this issue. We've rebased on top of the new
>> > state of master.
>> >
>> > We had initially kept the yarn.lock .gitignored, but ran into an issue
>> > rather early on where an upgraded dependency introduced a regression.
>> > Checking-in the yarn.lock provides the "know your dependency version"
>> > benefit of vendorizing code without vendorization's drawback of having
>> > to
>> > manually manage your dependencies.
>> >
>> > It is safe to delete a yarn.lock before applying a patch, as you are
>> > authoring master. It provides a history of the versions of each
>> > dependency
>> > that were working at the point in time of the commit. By contrast,
>> > package.json provides approximate versions and leaves room for
>> > fixes/improvements by the dependency authors to be pulled in as they
>> > become
>> > available.
>> >
>> > To run linter and tests:
>> >
>> > The tasks that Grunt used to manage are now defined as a set of scripts
>> > in
>> > the package.json
>> > After applying the patches--which may require deleting yarn.lock for the
>> > first patch--you should cd web && yarn install
>> >
>> > Then yarn test will run the linter, jasmine specs, and python tests
>> > including feature tests, in that order, exiting early if there are
>> > failures/errors.
>> > At the moment, the CheckForViewData test is failing on master as well as
>> > in
>> > each of these patches; that should be resolved as RM2477.
>> >
>> > Thanks!
>> > George and Matt
>> >
>> >
>> > On Thu, Jun 8, 2017 at 9:15 AM, Dave Page <dpage@pgadmin.org> wrote:
>> >>
>> >> Hi George
>> >>
>> >> On Wed, Jun 7, 2017 at 10:21 PM, George Gelashvili
>> >> <ggelashvili@pivotal.io> wrote:
>> >> > Hi Dave,
>> >> >
>> >> > I split the linting out into an intermediate commit, and rebased on
>> >> > top
>> >> > of
>> >> > master.
>> >>
>> >> Unfortunately, it still doesn't apply:
>> >>
>> >> error: patch failed: web/regression/javascript/test-main.js:1
>> >> error: removal patch leaves file contents
>> >> error: web/regression/javascript/test-main.js: patch does not apply
>> >> Checking patch web/regression/requirements.txt...
>> >> Checking patch web/webpack.config.js...
>> >> Checking patch web/webpack.test.config.js...
>> >> Checking patch web/yarn.lock...
>> >> error: web/yarn.lock: already exists in working directory
>> >> Applied patch .gitignore cleanly.
>> >> Applied patch Make.bat cleanly.
>> >> Applied patch README cleanly.
>> >> Applied patch pkg/mac/build.sh cleanly.
>> >> Applied patch pkg/pip/build.sh cleanly.
>> >> Applied patch pkg/src/build.sh cleanly.
>> >> Applied patch web/.eslintrc.js cleanly.
>> >> Applied patch web/karma.conf.js cleanly.
>> >> Applied patch web/package.json cleanly.
>> >> Applied patch web/pgAdmin4.py cleanly.
>> >> Applied patch web/pgadmin/static/jsx/components.jsx cleanly.
>> >> Applied patch web/pgadmin/tools/javascript/__init__.py cleanly.
>> >> Applied patch web/pgadmin/tools/javascript/javascript_bundler.py
>> >> cleanly.
>> >> Applied patch web/pgadmin/tools/javascript/tests/__init__.py cleanly.
>> >> Applied patch
>> >> web/pgadmin/tools/javascript/tests/test_javascript_bundler.py
>> >> cleanly.
>> >> Applied patch web/regression/README cleanly.
>> >> Applied patch
>> >> web/regression/javascript/jasmine_capture_warnings_beforeall.js
>> >> cleanly.
>> >> Applied patch web/regression/requirements.txt cleanly.
>> >> Applied patch web/webpack.config.js cleanly.
>> >> Applied patch web/webpack.test.config.js cleanly.
>> >>
>> >> The second (lint update) patch is even worse, with significant number
>> >> change that just don't want to apply.
>> >>
>> >> Clearly yarn.lock needs to be removed from there repo.
>> >>
>> >> Once I can apply a version of this, how should I be running the linter
>> >> and the unit tests?
>> >>
>> >> --
>> >> 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
>>
>> --
>> 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