public inbox for [email protected]
help / color / mirror / Atom feedFrom: Aditya Toshniwal <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: Khushboo Vashi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin][RM1802] ERD Tool (Beta)
Date: Mon, 18 Jan 2021 14:57:53 +0530
Message-ID: <CAM9w-_=Yn9wyEBxV2iBj5CLtyuqohLxD_263Z996jRNioXXGfQ@mail.gmail.com> (raw)
In-Reply-To: <CANxoLDcB4=x6qfL3HhW2E30c4N0pDLZtrKooMcocuRePM6v+sA@mail.gmail.com>
References: <CAM9w-_kowrenfKpiRLYWEpHJOOT2k9zRg6KCVpA0FR4Q=CA4hw@mail.gmail.com>
<CANxoLDda-CPNuU76FM1kOTdhNpRW170fr3FFQnaWxOpG=aeu2A@mail.gmail.com>
<CAFOhELd4JNuX82Vw3ZNdiKHHwBgu+rTdm9RfwY-qNjGkH47taQ@mail.gmail.com>
<CAFOhELdL0-M_itS4z-qNKUq6=RieWqc9UPos=oJTJFjyLrYQQw@mail.gmail.com>
<CAM9w-_kRdox8wvsHTwJ+vTOT2-kY1M4LvWFkm-799pEqJvHMsw@mail.gmail.com>
<CANxoLDcKjG-X=i7mrxVunS3oA+tBM3QmHgJN7y69CzT63y7a9A@mail.gmail.com>
<CAM9w-_=AFpLB-7aUVF2NZn4xRXe0o-LALJSjMsN8yBHqwd19fg@mail.gmail.com>
<CANxoLDcB4=x6qfL3HhW2E30c4N0pDLZtrKooMcocuRePM6v+sA@mail.gmail.com>
Hi,
The jasmine test cases are working fine on my local machine. The test cases
are successful on jenkins other than on linux, not sure why.
I have made some fixes by looking at the log. Please review and try.
On Mon, Jan 18, 2021 at 1:10 PM Akshay Joshi <[email protected]>
wrote:
> Thanks, patch applied.
>
> On Mon, Jan 18, 2021 at 10:34 AM Aditya Toshniwal <
> [email protected]> 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 <
>> [email protected]> wrote:
>>
>>> Thanks, patch applied.
>>>
>>> On Fri, Jan 15, 2021 at 7:01 PM Aditya Toshniwal <
>>> [email protected]> 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 <
>>>> [email protected]> 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 <
>>>>> [email protected]> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Dec 25, 2020 at 4:34 PM Akshay Joshi <
>>>>>> [email protected]> wrote:
>>>>>>
>>>>>>> Hi Khushboo,
>>>>>>>
>>>>>>> Can you please review it?
>>>>>>>
>>>>>>> On it.
>>>>>>
>>>>>>> On Fri, Dec 25, 2020 at 3:31 PM Aditya Toshniwal <
>>>>>>> [email protected]> 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*
>
--
Thanks,
Aditya Toshniwal
pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com*
<http://edbpostgres.com;
"Don't Complain about Heat, Plant a TREE"
Attachments:
[application/octet-stream] RM1801.jasmine.patch (1.5K, 3-RM1801.jasmine.patch)
download | inline diff:
diff --git a/web/regression/javascript/erd/onetomany_port_spec.js b/web/regression/javascript/erd/onetomany_port_spec.js
index 0ad4831ef..25c25937e 100644
--- a/web/regression/javascript/erd/onetomany_port_spec.js
+++ b/web/regression/javascript/erd/onetomany_port_spec.js
@@ -16,6 +16,6 @@ describe('ERD OneToManyPortModel', ()=>{
it('createLinkModel', ()=>{
let portObj = new OneToManyPortModel({options: {}});
- expect(portObj.createLinkModel()).toBeInstanceOf(OneToManyLinkModel);
+ expect(portObj.createLinkModel() instanceof OneToManyLinkModel).toBeTruthy();
});
});
diff --git a/web/regression/javascript/erd/table_node_spec.js b/web/regression/javascript/erd/table_node_spec.js
index d8edcdb44..1c8bf74ee 100644
--- a/web/regression/javascript/erd/table_node_spec.js
+++ b/web/regression/javascript/erd/table_node_spec.js
@@ -271,9 +271,9 @@ describe('ERD TableNodeWidget', ()=>{
it('icons', ()=>{
let cols = nodeWidget.find('.table-node .table-cols .col-row-data');
- expect(cols.at(0).find('.wcTabIcon').hasClass('icon-primary_key')).toBeTrue();
- expect(cols.at(1).find('.wcTabIcon').hasClass('icon-column')).toBeTrue();
- expect(cols.at(2).find('.wcTabIcon').hasClass('icon-column')).toBeTrue();
+ expect(cols.at(0).find('.wcTabIcon').hasClass('icon-primary_key')).toBeTruthy();
+ expect(cols.at(1).find('.wcTabIcon').hasClass('icon-column')).toBeTruthy();
+ expect(cols.at(2).find('.wcTabIcon').hasClass('icon-column')).toBeTruthy();
});
it('column names', ()=>{
view thread (21+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin][RM1802] ERD Tool (Beta)
In-Reply-To: <CAM9w-_=Yn9wyEBxV2iBj5CLtyuqohLxD_263Z996jRNioXXGfQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox