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 1l1P8x-0005jf-2t for pgadmin-hackers@arkaria.postgresql.org; Mon, 18 Jan 2021 07:40:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1l1P8w-0005HY-0h for pgadmin-hackers@arkaria.postgresql.org; Mon, 18 Jan 2021 07:40:06 +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 1l1P8v-0005HR-P7 for pgadmin-hackers@lists.postgresql.org; Mon, 18 Jan 2021 07:40:05 +0000 Received: from mail-io1-xd2e.google.com ([2607:f8b0:4864:20::d2e]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1l1P8s-0007ep-RK for pgadmin-hackers@postgresql.org; Mon, 18 Jan 2021 07:40:05 +0000 Received: by mail-io1-xd2e.google.com with SMTP id y19so31057328iov.2 for ; Sun, 17 Jan 2021 23:40:02 -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=v8TmKVSaSM6TavB8e51NrJTPaXYBpemRCy78HAmnl24=; b=UDOxg6yg0x9Iz+9cT7Nji4+PW9Wv8IJcBprSFkcJC42Wn4rfqk3vkyiw4czKj8ljJ6 6PPWETJE6PszQ8akxH2sbmMoSkw0lebmYyUjqoOoy2YkANMEEjGnl2lDUq/KiN+SjoGT vQcR5Fg5G1atB7qyfoDqIzYpQi/aaiI82VZq07Wmfq2yCREjLeF9h4T4CMeDqQiThOA4 TbHE40YR50zLacYSwOUI6QIkY0dJmYHzoarm9UJApaRm0xn194MdP4S9/W5p0Unzfj4U RDwPJX7Av4GKYFQyfPw66cmcTOLjBxM9b0Y2gawUAkMnvx7nvEzYKIw9QxVq97dMzGg1 DdEw== 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=v8TmKVSaSM6TavB8e51NrJTPaXYBpemRCy78HAmnl24=; b=a/u0gep4fbTecv/9csuNayZwug0Y1QMUcYN5wROAgIRjZfj6U2ZpdyVCL+kT+IdJQz hbHCBrTCyiEVxQufiiAVdCaHQ76nvWM95TYAoKgOCKuopccfhAy0SdeavcvzmqFSWn6v yo3CZqOO2sN/bcjJxueSNhcuOKYcoEd61FHQBgTFsKuDhQBehEnMWBnGzkNBW9iTxiH1 ShqvsCE0kgdFDKsYSuZRaO4K/USpWgjqE1hYsGUAevekTsv0xFTDsudohyRHDlAUEw2c I4+xO9xFifn4jyTFX81bEewEgyROXx1B6XgWkhwPdJXxMOpQ5FRWpw9WGe/sBAlGFrsD OLFg== X-Gm-Message-State: AOAM533Nc5wEUnzMRiPgH3HCoza2RV4daPfKWudp25wGAPVYVVq+7vx5 6pUZpyhbPBhtEGV45cYAElUElL1ufHT9O8lUbt7p8yMQLfTJwqticQWR/W+qkdMy3FOy+g7X9Eg Obk/HioenaYuSPaA0y7zjxD1HqE+XJdt3ZemFoYJop0YvDSjegBRBWJk9DvzTpRt2Qqvz0fgPoi k2AUzm95cK6XBDVYm3uR7eCICKbJqLpnjH909vfVn6JmpZEvYKo0wTF+jEOsklCqFfCA== X-Google-Smtp-Source: ABdhPJyIoHJylRGS4Qg6cJjsq5gFUOdzcKJN/+H2CgN+rIvRdsuhgqnYAn6WZ3oUrpGxvElZZlH6NZEMGrLBcQf/mdA= X-Received: by 2002:a6b:6a0e:: with SMTP id x14mr12340524iog.57.1610955601396; Sun, 17 Jan 2021 23:40:01 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Mon, 18 Jan 2021 13:09:50 +0530 Message-ID: Subject: Re: [pgAdmin][RM1802] ERD Tool (Beta) To: Aditya Toshniwal Cc: Khushboo Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000005ee08305b927d4d4" 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 --0000000000005ee08305b927d4d4 Content-Type: text/plain; charset="UTF-8" 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* --0000000000005ee08305b927d4d4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks, patch applied.

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

I forgot to re= move few of the dependencies which are not required as of now (may be in fu= ture). 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 Tosh= niwal <aditya.toshniwal@enterprisedb.com> wrote:
<= /blockquote>
Removed.=C2=A0
  • TableNode.jsx, below two lines can be combined.<= /li>
    import { PortModelAlignment, Def= aultNodeModel } from '@projectstorm/react-diagrams';
    import { Po= rtWidget } from '@projectstorm/react-diagrams';
    =
    • Done.=C2=A0
    • onImageClick fun=
ction in BodyWidget.jsx is no use I think, so it should be removed.<=
/li>
    I wanted to keep the code as it wi= ll be used in future. Anyway, I've removed the code.
    • I got some console errors while adding/editing tables. Refer to the attach=
ed screenshot.
    I tried but = I didn't get any. Looking at the screenshot, the error is from the unde= rlying library. Can'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 server is disconnected, a proper error message should be throw=
n, 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(B=
eta)...) as it is not opening a dialog.
    <= /blockquote>
    Done.=C2=A0
    • For large data sets, generate E=
RD hangs.
    It shows the sp= inner and waits for the response to come from the back end. I've used t= he existing table fetching code which is used at other places. I'll cre= ate an RM to improve the back end code for fetching the tables data which w= ill 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 h= elpful if we are using it frequently. ERD tool won't be used that frequ= ently.=C2=A0We already have a limited number of keys available for short= cuts. I think we should roll out without shortcut for now. If there is a us= er demand for it then we can think of adding it.
    =

    • SonarQu=
be fixes required.
    Fixed.=C2=A0
    Suggestion:
    <= /blockquote>
    Either we should warn the user OR make 2 differen=
t buttons for FK removal and table removal as the user may be confused if t=
he 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.
    = 
    Observa=
tions:
    Lodas= h has been used in this module in place of Underscore, though the dependenc= y is already introduced by another module,
    but we have mentioned it in t= he 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 can= not remove underscore because it is a dependency of backbone. Underscore is= outdated, and I cannot migrate the complete pgAdmin code.=C2=A0So, I de= cided 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 difficul= t 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 prope= rly detached from the tree itself, so we can reuse it.
    Yes. I agree. We need to separate out data source fr= om UI going forward=C2=A0with React.
    Thanks,
    Khushboo


    On Mon, Dec 28, 2020 at 10:53 AM Khushboo Vashi <khushboo.vashi@enterpris= edb.com> wrote:


    On Fri, Dec 25, 2020 at 4:34 PM Akshay Joshi <akshay.joshi@enterp= risedb.com> wrote:
    Hi Khush= boo,

    Can you please review it?

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

    Attached patch introduces ERD Tool(Beta) to pgAdmin. Below are the detai= ls:
    1) Create a diagram f= rom scratch or generate for an existing DB.
    2) Generate "Create" DDL from the diagram.
    3) Save the diagram and resu= me it later.
    4) Supports = basic table fields, one-to-many relationships, many-to-many relationships, = adding notes.
    5) Test cas= es added with 75-80% test coverage.

    Please re= view.

    --
    Thanks,
    Aditya Toshniwal
    pgAdmin hacker=C2=A0| Sr. Software Engineer | = edbpostgres.com
    "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"


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

    --0000000000005ee08305b927d4d4--