Re: [GSoC] Finalized First Patch

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 <yosrym93(at)gmail(dot)com> 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 <yosrym93(at)gmail(dot)com> 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