Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hfMlR-0002dM-VC for pgadmin-hackers@arkaria.postgresql.org; Mon, 24 Jun 2019 11:03:58 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hfMlQ-000547-Pt for pgadmin-hackers@arkaria.postgresql.org; Mon, 24 Jun 2019 11:03:56 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hfMlQ-000540-Cc for pgadmin-hackers@lists.postgresql.org; Mon, 24 Jun 2019 11:03:56 +0000 Received: from mail-lj1-x22a.google.com ([2a00:1450:4864:20::22a]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hfMlM-0005Ap-Vf for pgadmin-hackers@postgresql.org; Mon, 24 Jun 2019 11:03:55 +0000 Received: by mail-lj1-x22a.google.com with SMTP id r9so12169521ljg.5 for ; Mon, 24 Jun 2019 04:03:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=DwBAhc8cGCObd4+sGvgKEjv3tlNe5cBqpGF1GxoVRO8=; b=cnNkPDdzALlxf8WSckVG0dUUT5MpBDDxaitMk17Jvkc1S473deB08lQmoVccavYV1Z BnVM2i7Od036UJk7v1BuDamwn3QemN2+pKCFLCjM6xVPnjxaAihG8IBkRKEbqbsG1ZLI zsbbYdyC8OpN1MrLwGFNEZ+iN4MW6q7u6JTRrAaq/oFPSrYPWyULgUQjRh6Q0mtM+WOm k97o4D9W57t5IUi72f0ODoFpDQ7jOcGEGrHh677e7VomvTCgt4k/qxL+K3IQfxiONvpS NRNUnswagurqYIga73UwplY/6zwrxwFQpsgcYDC9QMZZQIFxAmvcUjQRMYgBcBJLpvkg /Hvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=DwBAhc8cGCObd4+sGvgKEjv3tlNe5cBqpGF1GxoVRO8=; b=T4/mCqZYowjrbTYY/sVP8nGmIcK2u/Oa3DQfEU5sSl7QVAeaaQZM6anQ9IoKTSSw6o HQ+a+yjPZ3ILjhCWBy98QXzAlrcr3/Dq4UcafGYenE5wmZUnaWhS8mIRYWjmFojs+Rdl jL921G65CQmPXzVlOPgi3ey0OFL0p5e5ugQjzMJagBUsVlvU7mQZmKSaCLY+j/ggwNlz +0nf0mffLus9h5qW8dNfrtDwPg4OeUGQ4oPl4xskYtXYexMCYOanYcPpcy/oT3/f98fQ W2qhoRt7TGRn91SRq4jKRhqNCjaHPtHYaLz3Dl9lzpj7yQbgyzBLzVZHx+2upu8j4JEp 5vuQ== X-Gm-Message-State: APjAAAWP+uYa2kKs97Xii26MCzBTgEmOfKWerLJoBSnxb/eMTqDptjIR WlBpgUCbHzczaR/q/9D15a0MRUi7AVf1Cxxs3vck6A== X-Google-Smtp-Source: APXvYqxaXrCpOTSNty76kf7PbytIg/ReMxibPurYCdRiZWdAHK6VCH0Onoq5aOeHBGAV40M1Nd2K9Shoy/fm4B0oGjU= X-Received: by 2002:a2e:7619:: with SMTP id r25mr2254276ljc.199.1561374231684; Mon, 24 Jun 2019 04:03:51 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Aditya Toshniwal Date: Mon, 24 Jun 2019 16:33:15 +0530 Message-ID: Subject: Re: [GSoC][Patch] Automatic Mode Detection V1 To: Yosry Muhammad Cc: Dave Page , pgadmin-hackers Content-Type: multipart/alternative; boundary="00000000000070fe12058c0fc44e" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000070fe12058c0fc44e Content-Type: text/plain; charset="UTF-8" Hi Yosry, On Mon, Jun 24, 2019 at 11:08 AM Yosry Muhammad wrote: > Hi all, > Please find attached a patch with all the changes (from the beginning of > the project). What I added in this patch: > > 1- Fixed #1 and #2 that Mr. Toshniwal pointed out in his code review. > Still waiting for a reply on #3. > What I meant for the color was to use existing colors defined which are based on a theme. The best fit for your work is $color-gray-lighter. Please use - $color-highlighted-grid-cell : $color-gray-lighter. Or may be remove $color-highlighted-grid-cell completely and use $color-gray-lighter in your class. > > 2- When data is edited in Query Tool with auto-commit turned off, the > notification message now tells the user that they need to commit these > changes. > > 3- The new icon is added and the functionality of both icons are now > completely separated as follows: > a) Save File button: exclusively for saving the query file (disabled > in View Table mode). > b) Save Data Changes button: for saving changes in data grid in both > modes. > I completely separated the 2 functionalities in all related files and > modules. I also fixed an existing bug that went as follows: > - The user has unsaved edits (existed in View Table mode). > - The user tries to close the panel, they are asked if they want to > save the changes. > - If they choose to save and the save failed (null in a non-null > column for example), the panel closes anyway. > The panel now does not close if the save failed. > > Something that is missing with the new button is the shortcut, I don't > know how to modify the Preferences in the configuration database. I could > not find the code responsible for adding data in the Preferences table and > so. Any help? > > 4- A savepoint is now created before any attempts are made to save data > changes, if the operation fails, the transaction is rolled back only till > the savepoint, keeping the previous SQL in the same transaction unharmed. > The whole transaction is rolled back if none existed in the first place. > > 5- Fixed a bug with all Alertiy.js confirm dialogs where line break would > break words. > > 6- I re-implemented the code responsible for handling the panel close > event in following way: > - The event used to handle one of two mutually exclusive events (or > neither): exiting with unsaved changes in the query or exiting with unsaved > changes in the data. > - As both can happen simultaneously now, I re-implemented this code to > check for multiple cases and produce sequential dialogs for different cases > (asynchronously to avoid freezing the page) . I also added a dialog that > asks for user confirmation when exiting with an un-commited transaction (or > data changes save). > > I have several questions: > - How can I edit the data in the configuration database (specifically the > preferences), what parts of the code are responsible for this? > In the sqleditor __init__.py, you'll find a call - RegisterQueryToolPreferences (web/pgadmin/tools/sqleditor/utils/query_tool_preferences.py). Refer the code for btn_save_file > - For running python tests, how should I produce an appropriate > test_config.json.in file for my environment? > Copy the test_config.json.in to test_config.json in the same directory. You just need to change the server details, sample below. You can add multiple servers to the list: ... "server_credentials": [ { "name": "PostgreSQL 9.4", "comment": "PostgreSQL 9.4 Server", "db_username": "postgres", "host": "localhost", "db_password": "postgres", "db_port": 5432, "maintenance_db": "postgres", "sslmode": "prefer", "tablespace_path": "", "enabled": true, "default_binary_paths": { "pg": "/Library/PostgreSQL/9.4/bin/", "ppas": "", "gpdb": "" } } ], ... > - After running python and feature tests, changes were made to nearly all > the files (git status shows modifications in a ton of files), is there > something I have done wrong? > What command did you use, can you share the screenshot of the files changed? > - When closing a panel in pgAdmin 4, my browser keeps asking me if I want > to leave the page or stay which I think might be annoying to some users > (specially when closing several tabs at once). We already produce dialogs > if any changes are unsaved, the browsers' ones are unnecessary. Is this > produces by our code or automatically by the browser? any way around it? I > use Firefox. > This can be disabled from Preferences -> Browser -> Display -> Confirm on close or refresh ?. Set it to false and you'll not get the browser warning. This is particularly helpful if you open the query tool in a new browser tab and close the tab itself. > - What else is missing from this patch to make it applicable ? I would > like to produce a release-ready patch if possible. If so, I can continue > working on the project on following patches, I just want to know what is > the minimum amount of work needed to make this patch release-ready > (especially that changes are being made in the master branch that require > me to re-edit parts of the code that I have written before to keep things > in-sync). > @Dave Page is the right person to answer this. > - For the bug that I reported before (generated queries in Query History > appear in a distorted way for the user), to get the actual query that is > being executed I can use the mogirfy() function of psycopg2 but I need > access to a cursor. I can get one directly in save_changed_data() function > by using conn.conn.cursor() but then I would be bypassing the wrapper > Connection class. Should I modify the wrapper Connection class and add a > function that can provide a cursor (or a wrapper around cursor.mogrify() )? > Thoughts? > Could you please share the query/screenshot ? The query history just stores the SQL text and fetches back to show in CodeMirror. No modifications/generation of queries is done by Query History. > > Here are things I think I might be working on next (share your thoughts): > - Make the transaction status more prominent. > - Handle cases where one or more columns can be made read-only for the > remaining of the resultset to be updatable (for example: SELECT col1, col2, > col1 || col2 as concat FROM some_table;). This will require modifying some > of the data that is sent from the backend to the frontend and a lot o > modifications (i think) in the front-end for handling columns individually. > > Thanks everyone and sorry for the long email ! > > On Thu, Jun 20, 2019 at 4:54 PM Yosry Muhammad wrote: > >> >> >> On Thu, Jun 20, 2019 at 7:49 AM Aditya Toshniwal < >> aditya.toshniwal@enterprisedb.com> wrote: >> >>> [forked the mail chain for code review] >>> Hi Yosry, >>> >>> On Wed, Jun 19, 2019 at 5:24 PM Dave Page wrote: >>> >>>> >>>> Aditya; can you do a quick code review please? Bear in mind it's a work >>>> in progress and there are no docs or tests etc. yet. >>>> >>> Nice work there. :) >>> >>> I just had look on the code changes, and have few suggestions: >>> 1) I found the code around primary key in the function >>> check_for_updatable_resultset_and_primary_keys repeating. Try if you >>> cpuld modify/reuse the get_primary_keys function. >>> 2) The function name check_for_updatable_resultset_and_primary_keys >>> could be shorter, something like check_updatabale_rset_pkeys. Same for >>> __set_updatable_resultset_attributes to __set_updatable_rset_attr >>> 3) You've used background: #f4f4f4; for .highlighted_grid_cells class. >>> This should go to sqleditor.scss with appropriate color from >>> web/pgadmin/static/scss/resources/_default.variables.scss. Hard coded color >>> codes are highly discouraged. >>> Otherwise, looks good (didn't run and check) >>> >>>> >>>> >>> >> I shortened both function names and fixed the hard-coded color. For #1, >> in the QueryToolCommand different processing of the primary keys occur in >> is_query_resultset_updatable function, where the attribute number of the >> primary keys is compared against columns and so. The only repeated part in >> check_for_updatable_resultset_and_primary_keys is the part where pk_names >> string is created in the required format (which is only a few lines of >> code). I could factor it out to a utility function - takes primary_keys >> dict and returns the pk_names string in the required format. What do you >> think? >> >> These changes (together with other changes that I am working on) will be >> included in the next version of this patch. >> >> Thanks ! >> >> >> -- >> *Yosry Muhammad Yosry* >> >> Computer Engineering student, >> The Faculty of Engineering, >> Cairo University (2021). >> Class representative of CMP 2021. >> https://www.linkedin.com/in/yosrym93/ >> > > > -- > *Yosry Muhammad Yosry* > > Computer Engineering student, > The Faculty of Engineering, > Cairo University (2021). > Class representative of CMP 2021. > https://www.linkedin.com/in/yosrym93/ > -- Thanks and Regards, Aditya Toshniwal Software Engineer | EnterpriseDB India | Pune "Don't Complain about Heat, Plant a TREE" --00000000000070fe12058c0fc44e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Yosry,

On Mon, Jun 24, 2019 at 11:08 A= M Yosry Muhammad <yosrym93@gmail.c= om> wrote:
Hi all,
Please find attached a patch with= all the changes (from the beginning of the project). What I added in this = patch:

1- Fixed #1 and #2 that Mr. Toshniwal point= ed out in his code review. Still waiting for a reply on #3.
What I meant for the color was to use existing colors defined whi= ch are based on a theme. The best fit for your work is=C2=A0$color-gray-lighter.=C2=A0Plea= se use - $color-highlighted-grid-= cell : $color-gray-lighter. Or may be remove= =C2=A0$color-h= ighlighted-grid-cell completely and use=C2=A0= $color-gray-li= ghter in your class.

2- When data is edited in Query Tool with auto-commit turned off, th= e notification message now tells the user that they need to commit these ch= anges.

3- The new icon is added and the functional= ity of both icons are now completely separated as follows:
=C2=A0= =C2=A0=C2=A0 a) Save File button: exclusively for saving the query file (di= sabled in View Table mode).
=C2=A0=C2=A0=C2=A0 b) Save Data C= hanges button: for saving changes in data grid in both modes.
I completely separated the 2 functionalities in all related files and modu= les. I also fixed an existing bug that went as follows:
=C2= =A0=C2=A0=C2=A0 - The user has unsaved edits (existed in View Table mode).<= br>
=C2=A0=C2=A0=C2=A0 - The user tries to close the panel, they = are asked if they want to save the changes.
=C2=A0=C2=A0=C2=A0 - = If they choose to save and the save failed (null in a non-null column for e= xample), the panel closes anyway.
The panel now does not clos= e if the save failed.

Something that is missing with the = new button is the shortcut, I don't know how to modify the Preferences = in the configuration database. I could not find the code responsible for ad= ding data in the Preferences table and so. Any help?

4- A= savepoint is now created before any attempts are made to save data changes= , if the operation fails, the transaction is rolled back only till the save= point, keeping the previous SQL in the same transaction unharmed. The whole= transaction is rolled back if none existed in the first place.
<= br>
5- Fixed a bug with all Alertiy.js confirm dialogs where line= break would break words.

6- I re-implemented the = code responsible for handling the panel close event in following way:
- The event used to handle one of two m= utually exclusive events (or neither): exiting with unsaved changes in the = query or exiting with unsaved changes in the data.
- As both can happen simultaneously now, I re-implemented this= code to check for multiple cases and produce sequential dialogs for differ= ent cases (asynchronously to avoid freezing the page) . I also added a dial= og that asks for user confirmation when exiting with an un-commited transac= tion (or data changes save).

I have several questions:
- How can I edit the data in the = configuration database (specifically the preferences), what parts of the co= de are responsible for this?
In the sqleditor __in= it__.py, you'll find a call -=C2=A0RegisterQueryToolPreferences (web/pgadmin/tools/sqleditor/utils/qu= ery_tool_preferences.py). Refer the code for=C2=A0btn_save_file
- For running python tests= , how should I produce an appropriate test_config.json.in file for my environment?
<= /div>
Copy the test_con= fig.json.in to test_config.json in the same directory. You just need to= change the server details, sample below. You can add multiple servers to t= he list:
...

"= ;server_credentials": [
=C2=A0 {
=C2=A0 =C2=A0 "name":= "PostgreSQL 9.4",
=C2=A0 =C2=A0 "comment": "Po= stgreSQL 9.4 Server",
=C2=A0 =C2=A0 "db_username": "= postgres",
=C2=A0 =C2=A0 "host": "localhost",=C2=A0 =C2=A0 "db_password": "postgres",
=C2=A0 = =C2=A0 "db_port": 5432,
=C2=A0 =C2=A0 "maintenance_db&quo= t;: "postgres",
=C2=A0 =C2=A0 "sslmode": "prefe= r",
=C2=A0 =C2=A0 "tablespace_path": "",
=C2= =A0 =C2=A0 "enabled": true,
=C2=A0 =C2=A0 "default_binary= _paths": {
=C2=A0 =C2=A0 =C2=A0 "pg": "/Library/Post= greSQL/9.4/bin/",
=C2=A0 =C2=A0 =C2=A0 "ppas": "&quo= t;,
=C2=A0 =C2=A0 =C2=A0 "gpdb": ""
=C2=A0 =C2=A0= }
=C2=A0 }
],

...
=C2=A0
- After running pyth= on and feature tests, changes were made to nearly all the files (git status= shows modifications in a ton of files), is there something I have done wro= ng?
What command did you use, can you share the sc= reenshot of the files changed?=C2=A0
- When closing a panel in pgA= dmin 4, my browser keeps asking me if I want to leave the page or stay whic= h I think might be annoying to some users (specially when closing several t= abs at once). We already produce dialogs if any changes are unsaved, the br= owsers' ones are unnecessary. Is this produces by our code or automatic= ally by the browser? any way around it? I use Firefox.
This can be disabled from Preferences -> =C2=A0Browser -> D= isplay -> Confirm on close or = refresh ?. Set it to fal= se and you'll not get the browser warning.
This is particularly helpful if you = open the query tool in a new browser tab and close the tab itself.
- What else is missing from this patch to make it applicable ? I wo= uld like to produce a release-ready patch if possible. If so, I can continu= e working on the project on following patches, I just want to know what is = the minimum amount of work needed to make this patch release-ready (especia= lly that changes are being made in the master branch that require me to re-= edit parts of the code that I have written before to keep things in-sync).<= /div>
@Dave Page is the right person to answer this.=C2=A0
- For the bug that I reported before (generated queries in Qu= ery History appear in a distorted way for the user), to get the actual quer= y that is being executed I can use the mogirfy() function of psycopg2 but I= need access to a cursor. I can get one directly in save_changed_data() fun= ction by using conn.conn.cursor() but then I would be bypassing the wrapper= Connection class. Should I modify the wrapper Connection class and add a f= unction that can provide a cursor (or a wrapper around cursor.mogrify() )? = Thoughts?
Could you please share the query/scr= eenshot ? The query history just stores the SQL text and fetches back to sh= ow in CodeMirror. No modifications/generation of queries is done by Query H= istory.

Here are things I think I might = be working on next (share your thoughts):
- Make the transaction = status more prominent.
- Handle cases where one or more columns c= an be made read-only for the remaining of the resultset to be updatable (fo= r example: SELECT col1, col2, col1 || col2 as concat FROM some_table;). Thi= s will require modifying some of the data that is sent from the backend to = the frontend and a lot o modifications (i think) in the front-end for handl= ing columns individually.

Th= anks everyone and sorry for the long email !

On Thu, Jun 20, 2019= at 4:54 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

<= br>
On Thu,= Jun 20, 2019 at 7:49 AM Aditya Toshniwal <aditya.toshniwal@enterprisedb.com= > wrote:
=
[forked the mail chain for code review]
Hi Yosry,

On Wed, Jun 19, 2019 at 5:24 PM Dave Pa= ge <dpage@pgadmin= .org> wrote:

Aditya; can you do= a quick code review please? Bear in mind it's a work in progress and t= here are no docs or tests etc. yet.
Nice wor= k there. :)
=C2=A0
I just had look on the code ch= anges, and have few suggestions:
1) I found the code around pr= imary key in the function check_f= or_updatable_resultset_and_primary_keys repeating. Try if you cpuld modify/= reuse=C2=A0the get_primary_keys function.
2) The function name ch= eck_for_updatable_resultset_and_primary_keys could be shorter, something li= ke check_updatabale_rset_pkeys. Same for __set_updatable_resultset_attribut= es to __set_updatable_rset_attr
3) You've used background: #f4f4f4; = for .highlighted_grid_cells class. This should go to sqleditor.scss with ap= propriate color from web/pgadmin/static/scss/resources/_default.variables.s= css. Hard coded color codes are highly discouraged.
Otherwise, looks good (didn't run and check)<= /span>
=C2=A0

I shortened both function names = and fixed the hard-coded color. For #1, in the QueryToolCommand different p= rocessing of the primary keys occur in is_query_resultset_updatable functio= n, where the attribute number of the primary keys is compared against colum= ns and so. The only repeated part in check_for_updatable_resultset_and_prim= ary_keys is the part where pk_names string is created in the required forma= t (which is only a few lines of code). I could factor it out to a utility f= unction - takes primary_keys dict and returns the pk_names string in the re= quired format. What do you think?
=C2=A0
These chan= ges (together with other changes that I am working on) will be included in = the next version of this patch.

Thanks !
=


--
=
Yosry Muhammad Yosry

Computer Engineering student,
T= he Faculty of Engineering,
Cairo Univer= sity (2021).
Class representative of CM= P 2021.


--
Yosry Muhammad Yosry

Compute= r Engineering student,
The Faculty of = Engineering,
Cairo University (2021).
Class representative of CMP 2021.


--
Adi= tya Toshniwal
Software Engineer |=C2=A0EnterpriseDB India |=C2=A0Pune
"Don't Complain a= bout Heat, Plant a TREE"
--00000000000070fe12058c0fc44e--