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

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