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 1l0juK-0006xt-0B for pgadmin-hackers@arkaria.postgresql.org; Sat, 16 Jan 2021 11:38:16 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1l0juI-0000Aj-Uf for pgadmin-hackers@arkaria.postgresql.org; Sat, 16 Jan 2021 11:38:14 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1l0juI-0000Ac-Iu for pgadmin-hackers@lists.postgresql.org; Sat, 16 Jan 2021 11:38:14 +0000 Received: from mail-io1-xd33.google.com ([2607:f8b0:4864:20::d33]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1l0juB-0003JB-Me for pgadmin-hackers@postgresql.org; Sat, 16 Jan 2021 11:38:14 +0000 Received: by mail-io1-xd33.google.com with SMTP id n4so23319460iow.12 for ; Sat, 16 Jan 2021 03:38:06 -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=Mr5ZL5n1O+Z1vbSzNWvPPskToBkP5PCHz1rIFYVeX8U=; b=WJB7I9S4cQl0UGXrjWoKvpm3SIf71sHwe/xzQtnMM6PTY+clhJm3xp392IS43PKqdJ 4a80J7GSIeb+fI4ilyuHxY0OPSsV9ezkd7jjhVNp3/pdCUQD6wv+umllamHhCAEkKZyL d9q+Kny8agVk71Xt9+vtXr2IdNb1tAYglFiJsDjvb0XTNzTkjUP5uOryPHyGNlFr24lH fsnsv46TOI+UGay9TkZOgpKoj0kWuVtZUv/pacMa+RWVP4hGsZsBlWx2roHSY3Zp+lH0 XIqECT4r/YUX1dyAULiiw8GmzCa0aiPS/eWaecjvbU310K9MAouDm0pbGMKkXyU35Q8z uyPg== 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=Mr5ZL5n1O+Z1vbSzNWvPPskToBkP5PCHz1rIFYVeX8U=; b=HugA2RD5FBKGTO/E0A5l/VGy1iQf3j5lWrEcyD2uqeAlUnkG+c8QKy1pBJy4OUkOyx z15/528IOmOjmVczw/peEREdNbuEeEKw1wh82XmD0iu0hucwrZTMLfe/x08Iz/JZik6P PUft8RgEkvisRz5Z0vhMW5audhlL+mZs+1iapoHHFVRxqAvodoa4rCuakCNS0acbOLj4 g101mizM3cqBtiU6T0o3q1nrTaIdJBiGthxKHv+pptO3mfBOmldbRvWvCRWVaQv8aD7w gW13Qw3H4VvjELui3O0+IL93oDfTYTBI3bkwGMKAjwJCQDWhIiiA9azonqGjwV7Cl6Np D1JQ== X-Gm-Message-State: AOAM533F1z+J6ktsV7sUFygpQwud1/ebFG20zjxuKy/9FJcT6eFnG6xM qa3agfq6NM+tJoJmurwxRfkq7pSk3QCQ2Jf7SoijnRnBCmLoLwtIpyrolB5OwFOAwbS9hMdXNdL hEOepPOGsWqLmU3EFX3yzH3c8Iyhv1SHw+ejI+u4KjGg/q4DtHbcbJNhpkOdQOU4Jg2Yi/Utis6 OMP5/HjMbe2wgkstuceYHkm0/1k5433juBG0mVndiszWCCTwROhSURot9W8g== X-Google-Smtp-Source: ABdhPJx0Cl2LaxU+aPIYrBXAsnVxMZ9OHb2koAgADkntHhN9DKzBycKUuzkKTbz4ALTk1dwFMOsKyFSp73fMwMhXSiE= X-Received: by 2002:a6b:6a0e:: with SMTP id x14mr7728378iog.57.1610797084778; Sat, 16 Jan 2021 03:38:04 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Sat, 16 Jan 2021 17:07:53 +0530 Message-ID: Subject: Re: [pgAdmin][RM1802] ERD Tool (Beta) To: Aditya Toshniwal Cc: Khushboo Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000000b3d9905b902ec9b" 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 --0000000000000b3d9905b902ec9b Content-Type: text/plain; charset="UTF-8" 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* --0000000000000b3d9905b902ec9b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks, patch applied.

On Fri, Jan 15, 2021 at 7:01 PM Adit= ya Toshniwal <adity= a.toshniwal@enterprisedb.com> wrote:
Hi,

I've fixed the issues. You can find the comments i= nline.
I've also added PropTypes f= or the components for increased validation.

On Tue, Jan 12,= 2021 at 12:18 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrot= e:
Hi Aditya,
The functionalities and the code looks good to me, =
however some of the comments as below:
  • Correct the comments at so=
me places (3 occurrences found  in /erd/__init__.py)  which mention Schema diff instead of ERD.
    • =

Some comments in the JS/JSX file regarding components/fu=
nctions (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.=C2=A0
  • R=
emove commented code
# re= q_args =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
<= div>= Removed.=C2=A0
  • TableNode.jsx, below two lines can be combined.
import { PortModelAlignment, DefaultNodeModel=
 } from '@projectstorm/react-diagrams';
import { PortWidget } fr=
om '@projectstorm/react-diagrams';
<= /blockquote>
Done.=C2=A0
  • onImageClick function in BodyWi=
dget.jsx is no use I think, so it should be removed.
=
I wanted to keep the code as it will be used in f= uture. Anyway, I've removed the code.
  • I got some con=
sole errors while adding/editing tables. Refer to the attached screenshot.<=
/font>
I tried but I didn't ge= t any. Looking at the screenshot, the error is from the underlying library.= Can't do much in this.=C2=A0
  • In the column Edit Mod=
e, while deleting the primary key, it gives the error, which does not go aw=
ay with any further modifications.
Fixed.=C2=A0
  • While generating the SQL, if the se=
rver is disconnected, a proper error message should be thrown, right now so=
me 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 i=
s not opening a dialog.
Don= e.=C2=A0
  • For large data sets, generate ERD hangs.=
It shows the spinner and waits fo= r 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 impro= ve the back end code for fetching the tables data which will help the schem= a diff tool as well.
  • Opening the ERD panel in a new wind=
ow is not working, it opens in the same tab even if you have set the Prefer=
ence "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 u= sing it frequently. ERD tool won't be used that frequently.=C2= =A0W= e 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 require=
d.
Fixed.=C2=A0
Suggestion:
While removal =
of the FK link, If any of the table is selected, it is being deleted with F=
K 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 tabl= e is also removed with the FK.
I've added a confirmation dialog which will show the nu= mber of tables and links selected. This way user will know what he has sele= cted before deleting.
<=
font face=3D"arial, sans-serif">
Observations:
Lodash has been used = in this module in place of Underscore, though the dependency is already int= roduced by another module,
but we have mentioned it in the package.json = file, which is somewhat not convincing to me.
Lodash is more advanced th= an 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 pgAd= min) so it will come to package.json without choice. We cannot remove under= score because it is a dependency of backbone. Underscore is outdated, and I= cannot 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 ou= t underscore with time. Just like jQuery vs ReactJS.


Table di= alog 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 wh= ile implementing React in pgAdmin, the nodes should be properly detached fr= om the tree itself, so we can reuse it.
Yes. I agree. We need to separate out data source from UI going for= ward=C2=A0with React.

Thanks,
Khu= shboo


On Mon, De= c 28, 2020 at 10:53 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com>= wrote:
<= div dir=3D"ltr">


= On Fri, Dec 25, 2020 at 4:34 PM Akshay Joshi <akshay.joshi@enterprisedb.com&= gt; wrote:
Hi Khushboo,
Can you please review it?

On it.=C2=A0
On Fri, Dec 25, 2020 at 3:31 PM Aditya Toshniwal = <= aditya.toshniwal@enterprisedb.com> wrote:
Hi Hackers,

Attached patc= h introduces ERD Tool(Beta) to pgAdmin. Below are the details:
=
1) Create a diagram from scratch or g= enerate for an existing DB.
2) Generate "Create" DDL from the diagram.
3) Save the diagram and resume it later.
4) Supports basic table fiel= ds, one-to-many relationships, many-to-many relationships, adding notes.
5) Test cases added with 75= -80% test coverage.

<= /font>
Please review.

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


--
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"


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

--0000000000000b3d9905b902ec9b--