Thanks, patch applied.
On Mon, Jan 18, 2021 at 10:34 AM Aditya Toshniwal <
aditya.toshni...@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.jo...@enterprisedb.com> wrote:
>
>> Thanks, patch applied.
>>
>> On Fri, Jan 15, 2021 at 7:01 PM Aditya Toshniwal <
>> aditya.toshni...@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.va...@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.va...@enterprisedb.com> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Fri, Dec 25, 2020 at 4:34 PM Akshay Joshi <
>>>>> akshay.jo...@enterprisedb.com> wrote:
>>>>>
>>>>>> Hi Khushboo,
>>>>>>
>>>>>> Can you please review it?
>>>>>>
>>>>>> On it.
>>>>>
>>>>>> On Fri, Dec 25, 2020 at 3:31 PM Aditya Toshniwal <
>>>>>> aditya.toshni...@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*
>>>>>>> <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*
>>
>
>
> --
> 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*
