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 1hj41z-0003zN-HC for pgadmin-hackers@arkaria.postgresql.org; Thu, 04 Jul 2019 15:52:19 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hj41x-0003YC-9a for pgadmin-hackers@arkaria.postgresql.org; Thu, 04 Jul 2019 15:52:17 +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 1hj41w-0003Ro-Oo for pgadmin-hackers@lists.postgresql.org; Thu, 04 Jul 2019 15:52:17 +0000 Received: from mail-wm1-x342.google.com ([2a00:1450:4864:20::342]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hj41p-0008S6-5T for pgadmin-hackers@postgresql.org; Thu, 04 Jul 2019 15:52:16 +0000 Received: by mail-wm1-x342.google.com with SMTP id p74so3163751wme.4 for ; Thu, 04 Jul 2019 08:52:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=gCX6fwNA7JtKvjdOSMQFS3TlXfs2vmLIpK4/RZzQ86A=; b=Ku463bKfTA4ZcIqyS9wZNSlIuUln9qSIOWdFY7NiXpTiExyeQelif5vCzqGyUt9ejQ UeBN1UsL1h/Jzxv/E4Y6PfGML3/5Re6rWFVgszHiguwTptyyEZ6JSBr3mhcQarMxPFsq uIfkkayGT5K9a6p1VGzJUNxmVF0MYW9AASDutonTpXX7R0l4Ze/Kmd+ygkTS8z9hWGoi R933nDSkSa3DTdtHhyNrOg38DvxnbqUfgau0N4OA1ij8nGkxBTw/AT3KEaqS/a3c+ocG rlpEPqhKlIkCWDlzZLQYv53w17L9EPcYgXFxgQ9o0Czn1/wtM3D9ewFD4e7Zz8vt/Bwb UfKQ== 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=gCX6fwNA7JtKvjdOSMQFS3TlXfs2vmLIpK4/RZzQ86A=; b=NdPwH7Z2BcR/lYGJP1zLCxDHnkQqyO1lTk/7oii253yET6qilXgOI8p7WD6AWElM5K ewZGQ8Vs3BUWNozuhNapz82PXaFPyGrXUbNWCcjzuccfmchTiVTWnce9OLGzqTugw73I iHBFUvxCg9b3w1Nt+Xn/F5/4c1/rqzaeyh0hQflkdf6121Ndf7gXcjifpEa5wbmk47MU lhUptgFrvo3zo8xis/VE/usWSQ5/T4snI/krmWIebxbr82c/uA9XTQNx6M2S2w4UWoz+ fZVvYguChsFTCjLCv79+b+hcE5xiXHylOh0B4DV+fXuCNMKp2aRU0QIOP9dYKDM8amVm 7jGg== X-Gm-Message-State: APjAAAW/BaTB2iWnhdfwPoMTShDnH3ZsrqI6wGfAou7eBffpoUkSaMqK kCFqAg0XVpSWqnq3Fkkm1NMf36q9WI1nwI7L0Oa3XQ== X-Google-Smtp-Source: APXvYqwPoDFcXEMwiSzLKi6Hov16/Cd/0Q/9WAEgAFHU5LmcKxVl2n87H7YgxJ09dsbM1Yo07JiS2GGSrJMDt5tywXg= X-Received: by 2002:a1c:f018:: with SMTP id a24mr170890wmb.66.1562255528015; Thu, 04 Jul 2019 08:52:08 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Thu, 4 Jul 2019 16:51:56 +0100 Message-ID: Subject: Re: [GSoC] Finalized First Patch To: Yosry Muhammad Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000cbd60c058cdcf51a" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000cbd60c058cdcf51a Content-Type: text/plain; charset="UTF-8" Hi Apologies for the delay - I've finished travelling now. I found a few issues, mostly minor: - The patch won't apply with "git apply" and only partially applies with patch -p0. Please "git add" all your changes and new files in your repo, and then run "git diff --cached --binary", which should create a usable patch. You can then un-stage your changes. - execute_query_utils.py is somewhat lacking in file header and any comments. - There is commented-out code in sqleditor.js - "committed" is miss-spelt in a number of places (2 m's, 2 t's) - On reflection, I don't think the "Data saved successfully, you still need to commit changes to the database." is prominent enough - in testing, I found it very easy to miss. That might also be compounded by the fact that "Alert on uncommitted transactions?" doesn't seem to be working for me. I get the "Save text" prompt to save the query text, but if I say no to that, the Query Tool instance closes with no further warning, despite having a transaction in progress. All in all, it's looking very good though. On Wed, Jul 3, 2019 at 5:22 PM Yosry Muhammad wrote: > I updated the patch to be in sync with recent commits made (since I sent > the patch) and fix merge conflicts. > > Please find the new patch attached. Waiting for review. > > Thanks. > > On Mon, Jul 1, 2019 at 9:19 PM Yosry Muhammad wrote: > >> Dear all, >> >> This is a final version of the patch I have been working on since the >> beginning of the GSoC project with tests and documentation. >> >> This patch includes: >> >> Features: >> - Implementeing the first version detecting updatable resultsets. Saving >> edited data works the same way as View Data mode (with small changes). A >> resultset is updatable if: >> - All columns belong to the same table. >> - All primary keys are selected. >> - No duplicate columns. >> >> - Adding the new Save Data icon and its shortcut in preferences. The old >> Save icon is used exclusively for Saving files now. >> - Integrating saving data changes into the ongoing transaction (if any). >> The user is notified that they need to commit the changes if auto-commit is >> off. >> - A failed save of data changes rolls back the data changes only, does >> not rollback previous queries in the same transaction. >> - Alerting the user when Execute/Explain is clicked with unsaved changes >> in the grid. >> - Modified/New cells are now highlighted. >> - Alerting the user when exiting with uncommited transactions. >> - Re-implementing the on tab close event of the query tool as multiple >> dialogs may be required for: unsaved data changes, unsaved file changes & >> uncommited transactions (they can all be enabled/disabled in preferences). >> - Hiding queries generated by pgAdmin from query history (until they are >> substituted by a mogrified version and have a checkbox added to >> enable/disable them). >> >> Bug fixes: >> - Fixed a bug where exit on save would exit even if the save was not >> successful. >> - Fixed a bug where alertify confirm dialogs had midword break wrapping. >> >> Tests: >> - Python tests: >> - test_is_query_resultset_updatable: Tests that updatable resultsets >> are detected correctly. >> - test_save_changed_data: Tests that additions, deletions & updates >> are performed correctly on updatable resultsets. >> - Feature tests: >> - query_tool_journey_test (existing - extended): Test that when the >> query resultset is updatable the user can modify cells and add new rows >> (and vice versa). >> - Updated other feature tests to match the new icon. >> - JS tests: >> - Updated call_render_after_poll_specs.js & >> keyboard_shortcuts_specs.js to test updates in related parts of the code. >> >> I could not add JS tests to test that the new Save Data button is enabled >> when the user edits a cell as I cannot mimic the actions of editing the >> grid. However, the new button is now used in View/Edit data feature tests >> and it works correctly. >> I also could not add JS tests to check that when the resultset is >> updatable the grid should be editable as the current code in sqleditor.js >> is not testable and it will required a lot of refactoring of this file, but >> again, this is covered by feature tests. >> >> Documentation: >> - Updated editgrid.rst, query_tool.rst, query_tool_shortcuts.rst, >> preferences.rst & keyboard_shortcuts.rst to match the new changes. >> - Updated the following screenshots: query_output_data.png, >> query_tool.png & query_toolbar.png >> >> The patch passes all tests performed by "make check" command. >> >> Please review, looking forward to any comments or feedback. >> >> 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/ > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --000000000000cbd60c058cdcf51a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

A= pologies for the delay - I've finished travelling now.

I found a few issues, mostly minor:

- The= patch won't apply with "git apply" and only partially applie= s with patch -p0. Please "git add" all your changes and new files= in your repo, and then run "git diff --cached --binary", which s= hould create a usable patch. You can then un-stage your changes.

- e= xecute_query_utils.py is somewhat lacking in file header and any comments.<= br>
- There is commented-out code in sqleditor.js

- "committ= ed" is miss-spelt in a number of places (2 m's, 2 t's)

= - On reflection, I don't think the "Data saved successfully, you s= till need to commit changes to the database." is prominent enough - in= testing, I found it very easy to miss. That might also be compounded by th= e fact that "Alert on uncommitted transactions?" doesn't seem= to be working for me. I get the "Save text" prompt to save the q= uery text, but if I say no to that, the Query Tool instance closes with no = further warning, despite having a transaction in progress.
All in all, it's looking very good though.

On Wed, Jul 3, 2= 019 at 5:22 PM Yosry Muhammad <yosrym93@gmail.com> wrote:
I updated the patch = to be in sync with recent commits made (since I sent the patch) and fix mer= ge conflicts.

Please find the new patch attach= ed. Waiting for review.

Thanks.
On Mon, J= ul 1, 2019 at 9:19 PM Yosry Muhammad <yosrym93@gmail.com> wrote:
Dear all,

This is a final version of the patch I have been work= ing on since the beginning of the GSoC project with tests and documentation= .

This patch includes:

Fe= atures:
- Implementeing the first version detecting updatable= resultsets. Saving edited data works the same way as View Data mode (with = small changes). A resultset is updatable if:
=C2=A0=C2=A0=C2=A0 -= All columns belong to the same table.
=C2=A0=C2=A0=C2=A0 - All p= rimary keys are selected.
=C2=A0=C2=A0=C2=A0 - No duplicate colum= ns.

- Adding the new Save Data icon and its shortc= ut in preferences. The old Save icon is used exclusively for Saving files n= ow.
- Integrating saving data changes into the ongoing transactio= n (if any). The user is notified that they need to commit the changes if au= to-commit is off.
- A failed save of data changes rolls back = the data changes only, does not rollback previous queries in the same trans= action.
- Alerting the user when Execute/Explain is clicked with = unsaved changes in the grid.
- Modified/New cells are now highlig= hted.
- Alerting the user when exiting with uncommited transactions.
- Re-implementing the on tab close event of the query tool as multip= le dialogs may be required for: unsaved data changes, unsaved file changes = & uncommited transactions (they can all be enabled/disabled in preferen= ces).
- Hiding queries generated by pgAdmin from query history (until th= ey are substituted by a mogrified version and have a checkbox added to enab= le/disable them).

Bug fixes:
- F= ixed a bug where exit on save would exit even if the save was not successfu= l.
- Fixed a bug where alertify confirm dialogs had midword break= wrapping.

Tests:
- Python tests:
=C2=A0=C2=A0=C2=A0 - test_is_query_resultset_updatable: Tests that = updatable resultsets are detected correctly.
=C2=A0=C2=A0=C2= =A0 - test_save_changed_data: Tests that additions, deletions & updates= are performed correctly on updatable resultsets.
- Feature tests:
=
=C2=A0=C2=A0=C2=A0 - query_tool_journey_test (existing - extended): Te= st that when the query resultset is updatable the user can modify cells and= add new rows (and vice versa).
=C2=A0=C2=A0=C2=A0 - Updated othe= r feature tests to match the new icon.
- JS tests:
= =C2=A0=C2=A0=C2=A0 -=C2=A0 Updated call_render_after_poll_specs.js & ke= yboard_shortcuts_specs.js to test updates in related parts of the code.
=C2=A0=C2=A0
I could not add JS tests to test that th= e new Save Data button is enabled when the user edits a cell as I cannot mi= mic the actions of editing the grid. However, the new button is now used in= View/Edit data feature tests and it works correctly.
I also= could not add JS tests to check that when the resultset is updatable the g= rid should be editable as the current code in sqleditor.js is not testable = and it will required a lot of refactoring of this file, but again, this is = covered by feature tests.

Documentation:
- Updated editgrid.rst, query_tool.rst, query_tool_shortcuts.rst, preferen= ces.rst & keyboard_shortcuts.rst to match the new changes.
- = Updated the following screenshots: query_output_data.png, query_tool.png &a= mp; query_toolbar.png

The patch passes all tes= ts performed by "make check" command.

Pl= ease review, looking forward to any comments or feedback.

Thanks !

--
Yosry Muh= ammad Yosry

<= /div>
Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.


--
Yosry Muhammad Yosry<= /div>

Computer Engineering student,
The Faculty of Engineering,
Cai= ro University (2021).
Class representat= ive of CMP 2021.


--
Dave Page
Blog: http://pgsnake.blogspot.= com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The = Enterprise PostgreSQL Company
--000000000000cbd60c058cdcf51a--