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

Thanks, patch applied.

On Fri, Jan 15, 2021 at 7:01 PM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)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(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"
>

--
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Principal Software Architect*
*EDB Postgres <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*