Re: [pgAdmin4] [PATCH] History Tab rewrite in React

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.

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?

We will recreate the patches and send them ASAP.

Thanks
Shruti & Joao

On Mon, Jun 12, 2017 at 6:59 AM Dave Page <dpage(at)pgadmin(dot)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(at)pivotal(dot)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(at)pgadmin(dot)org> wrote:
> >>
> >> Hi George
> >>
> >> On Wed, Jun 7, 2017 at 10:21 PM, George Gelashvili
> >> <ggelashvili(at)pivotal(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>