Re: [pgAdmin][RM1802] ERD Tool (Beta)

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(dot)vashi(at)enterprisedb(dot)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(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>>
>>
>> On Fri, Dec 25, 2020 at 4:34 PM Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Khushboo,
>>>
>>> Can you please review it?
>>>
>>> On it.
>>
>>> On Fri, Dec 25, 2020 at 3:31 PM Aditya Toshniwal <
>>> aditya(dot)toshniwal(at)enterprisedb(dot)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*
>>>> <http://edbpostgres.com>
>>>> "Don't Complain about Heat, Plant a TREE"
>>>>
>>>
>>>
>>> --
>>> *Thanks & Regards*
>>> *Akshay Joshi*
>>> *pgAdmin Hacker | Principal Software Architect*
>>> *EDB Postgres <http://edbpostgres.com>*
>>>
>>> *Mobile: +91 976-788-8246*
>>>
>>

--
Thanks,
Aditya Toshniwal
pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com*
<http://edbpostgres.com>
"Don't Complain about Heat, Plant a TREE"