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

Hi Hackers,

Attached are the updated patches that apply on top of master.

Thanks,
Shruti & Joao

On Mon, Jun 12, 2017 at 10:44 AM Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi Shruti
>
> On Mon, Jun 12, 2017 at 3:24 PM, Shruti B Iyer <siyer(at)pivotal(dot)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(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
>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>