public inbox for [email protected]
help / color / mirror / Atom feedFrom: Khushboo Vashi <[email protected]>
To: Chethana Kumar <[email protected]>
Cc: Aditya Toshniwal <[email protected]>
Cc: Akshay Joshi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [Feature-3452] Schema diff tool with Table, View, Materialized View, Functions and Procedures
Date: Thu, 2 Jan 2020 14:33:49 +0530
Message-ID: <CAFOhELeyyiy_G9bng7+DyQRb=PJC9YiPhM=sVojfxEWNJsZ0oA@mail.gmail.com> (raw)
In-Reply-To: <CAH4-4WtvwEnyrSoLHTtF2_ATsXM+MmE0KzSQk9kFbCa1CyfbgA@mail.gmail.com>
References: <CANxoLDciD0Cdc0WbMZWhM=zr8q4iwUfWtBUiam0gaD-N7x2pgw@mail.gmail.com>
<CAM9w-_=HP9R9O7ShicGcVkHnpdm=3C=3WoNAAmA0JLH57xLyUg@mail.gmail.com>
<CAFOhELcZxofbpbH_NrTGX4m_RiRJMB45HOaSAEjfL3JTVHybxg@mail.gmail.com>
<CAH4-4WtvwEnyrSoLHTtF2_ATsXM+MmE0KzSQk9kFbCa1CyfbgA@mail.gmail.com>
On Thu, Jan 2, 2020 at 2:04 PM Chethana Kumar <
[email protected]> wrote:
> Hi Khushboo,
>
> On Thu, Jan 2, 2020 at 12:04 PM Khushboo Vashi <
> [email protected]> wrote:
>
>> Hi Aditya,
>>
>> Thanks for the review. Please find the inline response.
>> Also, the updated patch attached.
>>
>> On Fri, Dec 27, 2019 at 4:55 PM Aditya Toshniwal <
>> [email protected]> wrote:
>>
>>> Hi Akshay/Khushboo,
>>>
>>> I have few suggestions/questions for the attached patch:
>>>
>>> 1. Code like SchemaDiffRegistry('server', ServerNode) should be
>>> replaced with SSchemaDiffRegistry(ServerModule.NODE_TYPE, ServerNode)
>>>
>>> Done
>>
>>>
>>> 1. The variables return_ajax_response, only_sql, json_resp as far as
>>> I understood are similar. Can we have same var name everywhere ?
>>>
>>> only_sql is used when I need only SQL but not to be executed in the
>> backend. json_resp is used to have the plain text/json response.
>>
>>>
>>> 1. Remove commented code -
>>> web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
>>> -> get_sql_from_table_diff
>>>
>>> Done
>>
>>>
>>> 1. In
>>> web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
>>> -> fetch_tables - keys_to_remove is passed. How is it different from
>>> keys_to_ignore used at other places ?
>>>
>>> keys_to_remove used to remove the table properties before comparison as
>> we combine all child nodes of the table while comparing and keys_to_ignore
>> is to ignore the keys while comparing. We have also used the keys_to_ignore
>> in the table node itself.
>>
>>>
>>> 1. web/pgadmin/tools/schema_diff/__init__.py
>>> -> check_version_compatibility has hardcoded version numbers. Can we
>>> use get_version_mapping_directories from
>>> web/pgadmin/utils/versioned_template_loader.py ?
>>>
>>> I have checked the possibilities before using it in the schema diff. The
>> * purpose and the return values * are different for both the files.
>>
>>>
>>> 1. Rename .reallyHidden to .really-hidden
>>>
>>> Done
>>
>>>
>>> 1. CSS class #schema-diff-grid -> background: white; - hardcoded
>>> color can be changed to use $color-bg instead. Also use rem or px for -
>>> font-size: 9pt.
>>>
>>> Done
>>
>>>
>>> 1.
>>> 2. .slick-group-toggle.collapsed, .slick-group-toggle.expanded -
>>> svgs are not required. Font awesome has the icons. refer - .obj_properties
>>> .collapsed .caret::before.
>>>
>>> Done
>>
>>>
>>> 1.
>>> 2. In
>>> web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js ->
>>> formatNode - Appends can be avoided and formed in a single statement.
>>> + } else {
>>> + return $('<span></span>').append(
>>> + $('<span></span>', {
>>> + class: 'wcTabIcon ' + optimage,
>>> + })
>>> + ).append($('<span></span>').text(opt.text));
>>> + }
>>> +};
>>>
>>> Any harm in this approach?
>>
>>>
>>> 1.
>>> 2. In
>>> web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
>>> -> fetchData - We should not use async = false.
>>> + $.ajax({
>>> + async: false,
>>> + url: url,
>>> + })
>>> 3. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
>>> Use 'sources/window' - pgWindow.
>>>
>>>
>>> 1. + let preferences = (window.opener !== null) ?
>>> window.opener.pgAdmin.Browser.get_preferences_for_module('schema_diff') :
>>> window.top.pgAdmin.Browser.get_preferences_for_module('schema_diff');
>>>
>>> Done
>>
>>>
>>> 1. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
>>> Use map instead of for loop. It will also remove the sel_rows_data.push.
>>> will be helpfull in large data.
>>> + for (var row = 0; row < sel_rows.length; row++) {
>>> + let data = self.grid.getData().getItem(sel_rows[row]);
>>> +
>>> + if (data.type) {
>>> + let tmp_data = {
>>> + 'node_type': data.type,
>>> + 'source_oid': parseInt(data.oid, 10),
>>> + 'target_oid': parseInt(data.oid, 10),
>>> + 'comp_status': data.status,
>>> + };
>>> +
>>> + if(data.status && (data.status.toLowerCase() ==
>>> 'different' || data.status.toLowerCase() == 'identical')) {
>>> + tmp_data['target_oid'] = data.target_oid;
>>> + }
>>> + sel_rows_data.push(tmp_data);
>>> + }
>>> + }
>>> +
>>> + url_params['sel_rows'] = sel_rows_data;
>>>
>>> This is a debatable topic as there are pros and cons of both map and for
>> loop. Like, it's more readable if we use map and in case of for loop,
>> chrome and firefox will be more happy in terms of performance.
>>
>>>
>>> 1.
>>> 2. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -Are
>>> we doing anything to handle failure.
>>> + connect_database(server_id, db_id, callback) {
>>> + var url = url_for('schema_diff.connect_database', {'sid':
>>> server_id, 'did': db_id});
>>> + $.post(url)
>>> + .done(function(res) {
>>> + if (res.success && res.data) {
>>> + callback(res.data);
>>> + }
>>> + })
>>> + .fail(function() {
>>> + // Fail
>>> + });
>>> +
>>> + }
>>>
>>> Forgot to handle, now added.
>>
>>>
>>> 1.
>>> 2. As you've added a completely different function for
>>> connect_server, I would suggest to rename dlgServerPass to a different name
>>> to avoid conflict with existing dlgServerPass in server.js
>>> + if (!Alertify.dlgServerPass) {
>>> + Alertify.dialog('dlgServerPass', function factory() {
>>>
>>> As we open the schema diff in different frame, I think, this should not
>> be the issue.
>>
>>>
>>> 1.
>>> 2. Generate script does not work if pgAdmin opened in iframe.
>>> Iframes are used by tools like Katacoda.
>>> [image: Screenshot 2019-12-27 at 16.40.47.png]
>>>
>>> Fixed, good catch.
>>
>>>
>>> 1.
>>> 2. Comparing objects loader is not attached to DDL Comparison panel.
>>> [image: compare_overlay.png]
>>>
>>> Fixed.
>>
>>>
>>> 1.
>>> 2. Filter icon and Generate script icon size are different. Also
>>> change icons CSS to use font-icon. You can refer icons from sqleditor.
>>> [image: Screenshot 2019-12-27 at 12.18.00.png]
>>>
>>> The problem is, for the generate script icon, I have used the svg (as no
>> similar icon in font-awesome) whereas for Filter, font-awesome is used. I
>> can replace the Generate Script icon from font-awesome (can search for some
>> similar icon) if Chetana agrees.
>> @Chethana Kumar <[email protected]> , please have a look.
>>
>> https://fontawesome.com/v4.7.0/icon/file-code-o
>>
>
> We can use the above one as it looks appropriate to me.
>
Thanks, I will change it.
>
> Thanks,
> Chethana kumar
>
>
>>
>> https://fontawesome.com/v4.7.0/icon/file-text-o
>>
>>>
>>> 1.
>>>
>>> *The fetch_objects_to_compare function used in each node uses loop to
>>> fetch data. Although it is working for now, but I would suggest using bulk
>>> fetch nodes instead of looping through all the nodes one by one.*
>>>
>>
>> Can you please elaborate the approach which you are suggesting?
>>
>> Thanks,
>> Khushboo
>>
>>>
>>> On Fri, Dec 20, 2019 at 6:59 PM Akshay Joshi <
>>> [email protected]> wrote:
>>>
>>>> Hi Hackers,
>>>>
>>>> Attached is the implementation of the new feature Schema Diff Tool.
>>>> Initial work(backend code to compare the objects) has been done by me and
>>>> then most of the task has been completed by *Khushboo Vashi. *Sending
>>>> the patch on behalf of her*.*
>>>>
>>>> Currently, this tool only supports Tables, Views, Materialized Views,
>>>> Functions and Procedures node.
>>>>
>>>> Please review and test it thoroughly. Suggestions are welcome to
>>>> improve the tool.
>>>>
>>>> --
>>>> *Thanks & Regards*
>>>> *Akshay Joshi*
>>>>
>>>> *Sr. Software Architect*
>>>> *EnterpriseDB Software India Private Limited*
>>>> *Mobile: +91 976-788-8246*
>>>>
>>>
>>>
>>> --
>>> Thanks and Regards,
>>> Aditya Toshniwal
>>> pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
>>> "Don't Complain about Heat, Plant a TREE"
>>>
>>
>
> --
> Chethana Kumar
> Principal UI/UX Designer
> EnterpriseDB Corporation
>
>
> The Postgres Database Company
>
> P: +91 86981 57146
> www.enterprisedb.com
>
Attachments:
[image/png] Screenshot 2019-12-27 at 12.18.00.png (47.9K, 3-Screenshot%202019-12-27%20at%2012.18.00.png)
download | view image
[image/png] Screenshot 2019-12-27 at 16.40.47.png (395.3K, 4-Screenshot%202019-12-27%20at%2016.40.47.png)
download | view image
[image/png] compare_overlay.png (411.6K, 5-compare_overlay.png)
download | view image
view thread (10+ 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], [email protected]
Subject: Re: [Feature-3452] Schema diff tool with Table, View, Materialized View, Functions and Procedures
In-Reply-To: <CAFOhELeyyiy_G9bng7+DyQRb=PJC9YiPhM=sVojfxEWNJsZ0oA@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