Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1l1SQy-0005n0-DF for pgadmin-hackers@arkaria.postgresql.org; Mon, 18 Jan 2021 11:10:56 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1l1SQx-00086b-BC for pgadmin-hackers@arkaria.postgresql.org; Mon, 18 Jan 2021 11:10:55 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1l1SQw-00086T-Ss for pgadmin-hackers@lists.postgresql.org; Mon, 18 Jan 2021 11:10:55 +0000 Received: from mail-io1-xd36.google.com ([2607:f8b0:4864:20::d36]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1l1SQt-0006Q9-Mk for pgadmin-hackers@postgresql.org; Mon, 18 Jan 2021 11:10:53 +0000 Received: by mail-io1-xd36.google.com with SMTP id p72so7241222iod.12 for ; Mon, 18 Jan 2021 03:10:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=pZm6WzMRyQTZErZeXGaHYqMmOdlW+M6x6BkwuMMsZnM=; b=yURl+gtNgj3cjymSf8VwfFf8PDXZgivEXqI8MTICXyUrqKP02TD1aNlCqT/eIRpb3b lgLb95W3DY4UcT78TPSZOUKTJO2iclOAgOfJPVO7jSz+jGSphPC+ItDN+WX/XBg/Nrk6 F/PmOzvoJDG4VfmZ3rDCp0jw6nveIbOwY9mTl83eLNEkCOvCTl/OSCp91z8hkOR+dgAp wxQpsklHQxo8kdrIp4yQt3+5GhxkP8DYQrsONVWSFFlAwd4Ts0O4o6LhooEEGqnwjlUg myF0xr5fsv32bexSwgnbt90wcUdF+3D6wd1CX2GKTQ2y5EJ2T5xnZldsGXywbPK9x1yp oy5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=pZm6WzMRyQTZErZeXGaHYqMmOdlW+M6x6BkwuMMsZnM=; b=FKMKhVfRBkQEwVhIg/MeAyEu7tm7qh2gnlTX+d5kAEjHhel1SlnZFI0TCsgmJFrTBM SzF14CE2dDE8rnsLJh4vovpcfg3FgwWMsWOzwWBFHR/DNonc7GcM4DjQmIalR1S2OisQ BeuzsBUSXoMjQJAkp4WU3g6T3v40IaGYyBjw+nQpEaL4x8hDGKmYdQcC+qVyeMKEocj8 0pq+zMAKBcVWixbfX7r/GPEiR1a6JHu9sDWHCbZDWtbFH1BSaP6XJVPCVqks8cBaJf7e +Gus0Th8DPhozdA82kWTyORsV+muLHWf7mThR7S9im8in8oXWS2+H2M8hxXOMEpBBmDh jtig== X-Gm-Message-State: AOAM533UeDXno1uJGPxwvolRudpmQpU4nPkd/BvRiZFR+2A6QYkdTFM4 tqgCVmaj8nAzngWi9nawqTuDZV4BjTAq5H0ZbJ9b+QGYIdlU07xPPlNUz1OT5gw0WC+A0G9nGgS FTUvy1BD76svdXND7rxCyC2eKucQXEEa+veoPZvDbhwwMwo0/8OvodOfNZ/i53zZ1BoOV9uaUa5 3OpDXkQmHG2wsbsIDkoJg+5HPxRoSlzDZRKNUtgN2roGXrw9wTP9FxYv7Mdw== X-Google-Smtp-Source: ABdhPJxmW9UWAjMTSUcx+eEqXTGa71P0dCfcpyybxulPZNvlfd6OzPjZ9837ta/Y96fw+vv5Jxxldnp+wrrka5Ndz0M= X-Received: by 2002:a05:6e02:1985:: with SMTP id g5mr20232584ilf.257.1610968250775; Mon, 18 Jan 2021 03:10:50 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Mon, 18 Jan 2021 16:40:39 +0530 Message-ID: Subject: Re: [pgAdmin][RM1802] ERD Tool (Beta) To: Aditya Toshniwal Cc: Khushboo Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000005514e705b92ac6fb" X-CLOUD-SEC-AV-Info: enterprisedb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000005514e705b92ac6fb Content-Type: text/plain; charset="UTF-8" Thanks, patch applied. On Mon, Jan 18, 2021 at 2:58 PM Aditya Toshniwal < aditya.toshniwal@enterprisedb.com> wrote: > Hi, > > The jasmine test cases are working fine on my local machine. The test > cases are successful on jenkins other than on linux, not sure why. > I have made some fixes by looking at the log. Please review and try. > > On Mon, Jan 18, 2021 at 1:10 PM Akshay Joshi < > akshay.joshi@enterprisedb.com> wrote: > >> Thanks, patch applied. >> >> On Mon, Jan 18, 2021 at 10:34 AM Aditya Toshniwal < >> aditya.toshniwal@enterprisedb.com> wrote: >> >>> Hi Akshay, >>> >>> I forgot to remove few of the dependencies which are not required as of >>> now (may be in future). Attached patch removes those dependencies from >>> package.json. >>> >>> On Sat, Jan 16, 2021 at 5:08 PM Akshay Joshi < >>> akshay.joshi@enterprisedb.com> wrote: >>> >>>> Thanks, patch applied. >>>> >>>> On Fri, Jan 15, 2021 at 7:01 PM Aditya Toshniwal < >>>> aditya.toshniwal@enterprisedb.com> wrote: >>>> >>>>> Hi, >>>>> >>>>> I've fixed the issues. You can find the comments inline. >>>>> I've also added PropTypes for the components for increased validation. >>>>> >>>>> On Tue, Jan 12, 2021 at 12:18 PM Khushboo Vashi < >>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>> >>>>>> Hi Aditya, >>>>>> >>>>>> The functionalities and the code looks good to me, however some of the comments as below: >>>>>> >>>>>> >>>>>> - Correct the comments at some places (3 occurrences found in /erd/__init__.py) which mention Schema diff instead of ERD. >>>>>> >>>>>> Some comments in the JS/JSX file regarding components/functions (For example, IconButton (forwardRef), Bodywidget etc.) would >>>>>> >>>>>> be great help as we all are new to React. >>>>>> >>>>>> Done. Added comments to the components. >>>>> >>>>>> >>>>>> - Remove the unused imports (for ex bad_request) in /erd/__init__.py >>>>>> >>>>>> Removed. >>>>> >>>>>> >>>>>> - Remove commented code >>>>>> >>>>>> # req_args = request.args >>>>>> # if ('recreate' in req_args and >>>>>> # req_args['recreate'] == '1'): >>>>>> # connect = False >>>>>> >>>>>> Removed. >>>>> >>>>>> >>>>>> - TableNode.jsx, below two lines can be combined. >>>>>> >>>>>> import { PortModelAlignment, DefaultNodeModel } from >>>>>> '@projectstorm/react-diagrams'; >>>>>> import { PortWidget } from '@projectstorm/react-diagrams'; >>>>>> >>>>>> Done. >>>>> >>>>>> >>>>>> - onImageClick function in BodyWidget.jsx is no use I think, so it should be removed. >>>>>> >>>>>> I wanted to keep the code as it will be used in future. Anyway, I've >>>>> removed the code. >>>>> >>>>>> >>>>>> - I got some console errors while adding/editing tables. Refer to the attached screenshot. >>>>>> >>>>>> I tried but I didn't get any. Looking at the screenshot, the error is >>>>> from the underlying library. Can't do much in this. >>>>> >>>>>> >>>>>> - In the column Edit Mode, while deleting the primary key, it gives the error, which does not go away with any further modifications. >>>>>> >>>>>> Fixed. >>>>> >>>>>> >>>>>> - While generating the SQL, if the server is disconnected, a proper error message should be thrown, right now some server side error is coming. >>>>>> >>>>>> It will show connection lost error now. Fixed. >>>>> >>>>>> >>>>>> - Please remove ... from the menu title (New ERD Project(Beta)...) as it is not opening a dialog. >>>>>> >>>>>> Done. >>>>> >>>>>> >>>>>> - For large data sets, generate ERD hangs. >>>>>> >>>>>> It shows the spinner and waits for the response to come from the back >>>>> end. I've used the existing table fetching code which is used at other >>>>> places. I'll create an RM to improve the back end code for fetching the >>>>> tables data which will help the schema diff tool as well. >>>>> >>>>>> >>>>>> - Opening the ERD panel in a new window is not working, it opens in the same tab even if you have set the Preference "Open in new browser tab" to True. >>>>>> >>>>>> Fixed. Added the setting in "Tab settings". >>>>> >>>>>> >>>>>> - No shortcut is provided to open the ERD Tool. >>>>>> >>>>>> A shortcut is helpful if we are using it frequently. ERD tool won't >>>>> be used that frequently. We already have a limited number of keys >>>>> available for shortcuts. I think we should roll out without shortcut for >>>>> now. If there is a user demand for it then we can think of adding it. >>>>> >>>>>> >>>>>> - SonarQube fixes required. >>>>>> >>>>>> Fixed. >>>>> >>>>>> >>>>>> - >>>>>> >>>>>> *Suggestion:* >>>>>> >>>>>> While removal of the FK link, If any of the table is selected, it is being deleted with FK link. >>>>>> Either we should warn the user OR make 2 different buttons for FK removal and table removal as the user may be confused if the selected table is also removed with the FK. >>>>>> >>>>>> I've added a confirmation dialog which will show the number of tables >>>>> and links selected. This way user will know what he has selected before >>>>> deleting. >>>>> >>>>>> *Observations:* >>>>>> >>>>>> Lodash has been used in this module in place of Underscore, though >>>>>> the dependency is already introduced by another module, >>>>>> but we have mentioned it in the package.json file, which is somewhat >>>>>> not convincing to me. >>>>>> Lodash is more advanced than Underscore but we should pick anyone as >>>>>> it will be easy to manage. >>>>>> >>>>>> TL;DR; we cannot. >>>>> lodash is a peer dependency for react-diagrams (and some existing >>>>> modules in pgAdmin) so it will come to package.json without choice. We >>>>> cannot remove underscore because it is a dependency of backbone. Underscore >>>>> is outdated, and I cannot migrate the complete pgAdmin code. So, I >>>>> decided to go with 100/0 method. All the new codes will use lodash only as >>>>> we'll phase out underscore with time. Just like jQuery vs ReactJS. >>>>> >>>>>> >>>>>> >>>>>> Table dialog code is duplicate of the table node, as it was difficult >>>>>> to extend it because it was attached to the tree. >>>>>> So, we need to keep in mind that while implementing React in pgAdmin, >>>>>> the nodes should be properly detached from the tree itself, so we can reuse >>>>>> it. >>>>>> >>>>>> Yes. I agree. We need to separate out data source from UI going >>>>> forward with React. >>>>> >>>>>> >>>>>> Thanks, >>>>>> Khushboo >>>>>> >>>>>> >>>>>> On Mon, Dec 28, 2020 at 10:53 AM Khushboo Vashi < >>>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Fri, Dec 25, 2020 at 4:34 PM Akshay Joshi < >>>>>>> akshay.joshi@enterprisedb.com> wrote: >>>>>>> >>>>>>>> Hi Khushboo, >>>>>>>> >>>>>>>> Can you please review it? >>>>>>>> >>>>>>>> On it. >>>>>>> >>>>>>>> On Fri, Dec 25, 2020 at 3:31 PM Aditya Toshniwal < >>>>>>>> aditya.toshniwal@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi Hackers, >>>>>>>>> >>>>>>>>> Attached patch introduces ERD Tool(Beta) to pgAdmin. Below are the >>>>>>>>> details: >>>>>>>>> 1) Create a diagram from scratch or generate for an existing DB. >>>>>>>>> 2) Generate "Create" DDL from the diagram. >>>>>>>>> 3) Save the diagram and resume it later. >>>>>>>>> 4) Supports basic table fields, one-to-many relationships, >>>>>>>>> many-to-many relationships, adding notes. >>>>>>>>> 5) Test cases added with 75-80% test coverage. >>>>>>>>> >>>>>>>>> Please review. >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Thanks, >>>>>>>>> Aditya Toshniwal >>>>>>>>> pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com* >>>>>>>>> >>>>>>>>> "Don't Complain about Heat, Plant a TREE" >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> *Thanks & Regards* >>>>>>>> *Akshay Joshi* >>>>>>>> *pgAdmin Hacker | Principal Software Architect* >>>>>>>> *EDB Postgres * >>>>>>>> >>>>>>>> *Mobile: +91 976-788-8246* >>>>>>>> >>>>>>> >>>>> >>>>> -- >>>>> Thanks, >>>>> Aditya Toshniwal >>>>> pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com* >>>>> >>>>> "Don't Complain about Heat, Plant a TREE" >>>>> >>>> >>>> >>>> -- >>>> *Thanks & Regards* >>>> *Akshay Joshi* >>>> *pgAdmin Hacker | Principal Software Architect* >>>> *EDB Postgres * >>>> >>>> *Mobile: +91 976-788-8246* >>>> >>> >>> >>> -- >>> Thanks, >>> Aditya Toshniwal >>> pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com* >>> >>> "Don't Complain about Heat, Plant a TREE" >>> >> >> >> -- >> *Thanks & Regards* >> *Akshay Joshi* >> *pgAdmin Hacker | Principal Software Architect* >> *EDB Postgres * >> >> *Mobile: +91 976-788-8246* >> > > > -- > Thanks, > Aditya Toshniwal > pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com* > > "Don't Complain about Heat, Plant a TREE" > -- *Thanks & Regards* *Akshay Joshi* *pgAdmin Hacker | Principal Software Architect* *EDB Postgres * *Mobile: +91 976-788-8246* --0000000000005514e705b92ac6fb Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks, patch applied.

On Mon, Jan 18, 2021 at 2:58 PM Adit= ya Toshniwal <adity= a.toshniwal@enterprisedb.com> wrote:
Hi,

The jasmine test cases are= working=C2=A0fine on my local machine. The test cases are successful on je= nkins other than on linux, not sure why.
I have made some fixes by looking = at the log. Please review and try.

On Mon, Jan 18, 2021 at 1:10 PM Aks= hay Joshi <akshay.joshi@enterprisedb.com> wrote:
Thanks, patch applied.<= br>

On Mon, Jan 18, 2021 at 10:34 AM Aditya Toshniwal <aditya.toshniwal@enter= prisedb.com> wrote:
Hi Akshay,

I forgot to remove few of the depe= ndencies which are not required as of now (may be in future). Attached patc= h removes those dependencies from package.json.

On Sat, Jan 16, 2021= at 5:08 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
=
Thanks, = patch applied.

On Fri, Jan 15, 2021 at 7:01 PM Aditya Toshniwal <aditya.tos= hniwal@enterprisedb.com> wrote:
Hi,

I've fixed the issues. You can find the comments inline= .
I've also added PropTypes for th= e components for increased validation.

On Tue, Jan 12, 2021= at 12:18 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
<= div dir=3D"ltr">
Hi Aditya,
The functionalities and the code looks good to me, howev=
er some of the comments as below:
  • Correct the comments at some pl=
aces (3 occurrences found  in /e=
rd/__init__.py)  which mention Schema diff instead of ERD.
=

Some comments in the JS/JSX file regarding components/functio=
ns (For example, IconButton (forwardRef), Bodywidget etc.) would
be great=
 help as we all are new to React.
Done. Added comments to the components.
  • Remov= e the unused imports (for ex bad_request) in /erd/__init__.py
Removed.=C2=A0
  • Remov= e commented code
# req_ar= gs =3D request.args
# if ('recreate' in req_args and
# =C2=A0= =C2=A0 req_args['recreate'] =3D=3D '1'):
# =C2=A0 =C2= =A0 connect =3D False
= Remo= ved.=C2=A0
  • TableNode.jsx, below two lines can be combined.
import { PortModelAlignment, DefaultNodeModel } f= rom '@projectstorm/react-diagrams';
import { PortWidget } from &= #39;@projectstorm/react-diagrams';
Done.=C2=A0
  • onImageClick function in BodyWidget=
.jsx is no use I think, so it should be removed.
I wanted to keep the code as it will be used in futur= e. Anyway, I've removed the code.
  • I got some conso=
le errors while adding/editing tables. Refer to the attached screenshot.
I tried but I didn't get = any. Looking at the screenshot, the error is from the underlying library. C= an't do much in this.=C2=A0
  • In the column Edit Mode,=
 while deleting the primary key, it gives the error, which does not go away=
 with any further modifications.
Fixed.=C2=A0
  • While generating the SQL, if the serv=
er is disconnected, a proper error message should be thrown, right now some=
 server side error is coming.
It will show connection lost error now. Fixed.=C2=A0
  • Please remove ... from the menu title (New ERD Project(Beta)...) as it is =
not opening a dialog.
<= span class=3D"gmail_default" style=3D"font-family:verdana,sans-serif">Done.= =C2=A0
  • For large data sets, generate ERD hangs.
It shows the spinner and waits for = the response to come from the back end. I've used the existing table fe= tching code which is used at other places. I'll create an RM to improve= the back end code for fetching the tables data which will help the schema = diff tool as well.
  • Opening the ERD panel in a new window=
 is not working, it opens in the same tab even if you have set the Preferen=
ce "Open in new browser tab" to True.
Fixed. Added the setting in "Tab settings".<= /span>
  • No shortcut is provided to open the ERD Tool.
A shortcut is helpful if we are using= it frequently. ERD tool won't be used that frequently.=C2=A0We alre= ady have a limited number of keys available for shortcuts. I think we shoul= d roll out without shortcut for now. If there is a user demand for it then = we can think of adding it.
  • SonarQube fixes required.
Fixed.=C2=A0
  • Suggestion:
While removal of the=
 FK link, If any of the table is selected, it is being deleted with FK link=
.
Either we should warn the user OR make 2 different buttons for FK remov=
al and table removal as the user may be confused if the selected table is a=
lso removed with the FK.
I've added a confirmation dialog which will show the number o= f tables and links selected. This way user will know what he has selected b= efore deleting.
Observations:
Lodash has been used in thi= s module in place of Underscore, though the dependency is already introduce= d by another module,
but we have mentioned it in the package.json file, = which is somewhat not convincing to me.
Lodash is more advanced than Und= erscore but we should pick anyone as it will be easy to manage.
<= /blockquote>
TL;DR; we cannot.
lodash is a = peer dependency for react-diagrams (and some existing modules in pgAdmin) s= o it will come to package.json without choice. We cannot remove underscore = because it is a dependency of backbone. Underscore is outdated, and I canno= t migrate the complete pgAdmin code.=C2=A0So, I decided to go with 100/0= method. All the new codes will use lodash only as we'll phase out unde= rscore with time. Just like jQuery vs ReactJS.


Table dialog co= de is duplicate of the table node, as it was difficult to extend it because= it was attached to the tree.
So, we need to keep in mind that while imp= lementing React in pgAdmin, the nodes should be properly detached from the = tree itself, so we can reuse it.
=
Yes. I agree. We need to separate out data source from UI going forward=C2=A0with React.

Thanks,
Khushboo

On Mon, Dec 28, 2= 020 at 10:53 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:=


On Fri,= Dec 25, 2020 at 4:34 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wro= te:
Hi Khushboo,

Can you please review it?

On i= t.=C2=A0
On Fri, Dec 25, 2020 at 3:31 PM Aditya Toshniwal <aditya.= toshniwal@enterprisedb.com> wrote:
Hi Hackers,
=
Attached patch intro= duces ERD Tool(Beta) to pgAdmin. Below are the details:
1) Create a diagram from scratch or generate= for an existing DB.
2) G= enerate "Create" DDL from the diagram.
3) Save the diagram and resume it later.
4) Supports basic table fields, one= -to-many relationships, many-to-many relationships, adding notes.
5) Test cases added with 75-80% te= st coverage.

<= /div>
Please review.
=
--
Thanks,
Aditya Toshniwal
pgAdmin hacker=C2=A0| Sr. Software Eng= ineer | edbpostgres.com<= /font>
"Don't Complain about Heat, Plant a TREE"


--
Thanks & Regards
Akshay Joshi
= pgAdmin Hacker | Principal Software Ar= chitect
EDB Po= stgres
Mobile: +91 976-788-8246

<= /div>


--
Thanks,
Aditya Toshniwal=
pgAdmin hacker=C2=A0| Sr. Software Engineer | edbpostgres.com
&quo= t;Don't Complain about Heat, Plant a TREE"


--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Principal Softw= are Architect
EDB Po= stgres
Mobile: +91 976-788-8246



--
Thanks,
Aditya Toshniwal=
pgAdmin hacker=C2=A0| Sr. Software Engineer | edbpostgres.com
&quo= t;Don't Complain about Heat, Plant a TREE"


--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Principal Softw= are Architect
EDB Po= stgres
Mobile: +91 976-788-8246



--
Thanks,
Aditya Toshniwal=
pgAdmin hacker=C2=A0| Sr. Software Engineer | edbpostgres.com
&quo= t;Don't Complain about Heat, Plant a TREE"


--
Thanks & Regards
Akshay Joshi
pgAdmi= n Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246

--0000000000005514e705b92ac6fb--