Обсуждение: [GSoC] Finalized First Patch

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/

Hi

On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad <yosrym93@gmail.com> wrote:

- 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.

That was due to new image. I made an applicable one with the below modifications, please find it attached.

Thanks!

 

 

- execute_query_utils.py is somewhat lacking in file header and any comments.

Added.

- There is commented-out code in sqleditor.js

This is the call that adds queries that are generated by pgAdmin to the query history. I commented it instead of removing it as I will add it later with some modifications when I add the checkbox.

Sure, but we won't commit commented-out code. It just makes things messy until such time as it gets used (or more often, does not).

 

- 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.

I made a separate notification for the uncommitted save to make it more visible, check it out.

That's definitely clearer now. It avoids the "blindness" caused by the fact that you always get the green message.

 

I tried the scenario you provided and the uncommitted alert worked fine, could you please try again and tell me the exact scenario where that happened?

Hmm, it's working fine for me today too. Definitely wasn't yesterday though!

I'm going to make some minor tweaks to the wording of the docs before I commit (as well as removing the commented-out code), but I think this is good to go, once it's had another review. Khushboo, please take a look as soon as you can.

Thanks! 

--

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Yosry,

I liked the way you have refactored the code at some places in the JS file and made it cleaner.

Here are some points:

1. The table (including partition table) with a single column having that column primary key is editable but the save button is disabled, so, ultimately I can't save the data. Note: The table should be empty to reproduce this issue.

2. command.py - The check_updatable_results_pkeys function calling the poll function and checks the ASYNC_OK, I think this is not required as this function is called from the poll function from the sqleditor/__init__.py *if the status of the polling is if ASYNC_OK*. So, I think this is overhead but if you have considered another scenario then let me know.

3. In the Preferences, the label of the keyboard shortcut "Save Data Changes" should be "Save data changes".

4. Dave has already mentioned about the commented code, so I do agree we should remove it.

5. I didn't find the doc updates for the keyboard shortcuts in the Preferences module as well as related to this feature. Am I missing something?

Keep doing wonderful work for pgAdmin. :)

Thanks,

Khushboo

 

On Fri, Jul 5, 2019 at 4:31 PM Dave Page <dpage@pgadmin.org> wrote:

Hi

On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad <yosrym93@gmail.com> wrote:

- 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.

That was due to new image. I made an applicable one with the below modifications, please find it attached.

Thanks!

 

 

- execute_query_utils.py is somewhat lacking in file header and any comments.

Added.

- There is commented-out code in sqleditor.js

This is the call that adds queries that are generated by pgAdmin to the query history. I commented it instead of removing it as I will add it later with some modifications when I add the checkbox.

Sure, but we won't commit commented-out code. It just makes things messy until such time as it gets used (or more often, does not).

 

- 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.

I made a separate notification for the uncommitted save to make it more visible, check it out.

That's definitely clearer now. It avoids the "blindness" caused by the fact that you always get the green message.

 

I tried the scenario you provided and the uncommitted alert worked fine, could you please try again and tell me the exact scenario where that happened?

Hmm, it's working fine for me today too. Definitely wasn't yesterday though!

I'm going to make some minor tweaks to the wording of the docs before I commit (as well as removing the commented-out code), but I think this is good to go, once it's had another review. Khushboo, please take a look as soon as you can.

Thanks! 

--

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Khushboo,

Please find an updated patch attached with the mentioned import line removed.

On Thu, Jul 11, 2019 at 6:45 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Hi,

On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Some points I missed:

1.  I assumed that in this patch modification in case of OIDs= True (without primary key) has not considered as that is not working.

This is not implemented yet. I will work on that in a following patch soon enough.

Okay. 

2. As we are already showing the changed Data prompt on closing the Query Tool, do we really need the Uncommitted Transaction prompt? 

This is needed when auto-commit is off. Saving changes in the data grid is performed as part of the ongoing transaction (or a new one if none is ongoing). After saving the data changes the user should still commit the current transaction for the changes to be commited to the database. This feature is also useful in general when auto-commit is off as users may forget to commit ongoing transactions.

One thing I have noticed, when I add a new row and delete it immediately without saving it and try to close the query tool, the uncommitted prompt is coming.

In my opinion, it should not come, what do you think?

We should disable the prompt if auto-commit and auto-rollback both are enabled.

The uncommited prompt does not keep track of what the user has done so far, it only checks for the current transaction status. If a current transaction is ongoing, the prompt comes up. If you added a new row then deleted it without saving, the transaction status is not affected, you must have done a previous operation and had auto-commit turned off (probably the select statement).

if auto-commit & auto-rollback are both enabled then there won't be any ongoing transaction at any point, thus, the prompt will never come up.

Looking forward for any 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/

On Fri, Jul 12, 2019 at 5:57 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi Yosry,

On Thu, Jul 11, 2019 at 6:50 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Hi Khushboo,

Please find an updated patch attached with the mentioned import line removed.

Looks good to me.  

On Thu, Jul 11, 2019 at 6:45 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Hi,

On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Some points I missed:

1.  I assumed that in this patch modification in case of OIDs= True (without primary key) has not considered as that is not working.

This is not implemented yet. I will work on that in a following patch soon enough.

Okay. 

2. As we are already showing the changed Data prompt on closing the Query Tool, do we really need the Uncommitted Transaction prompt? 

This is needed when auto-commit is off. Saving changes in the data grid is performed as part of the ongoing transaction (or a new one if none is ongoing). After saving the data changes the user should still commit the current transaction for the changes to be commited to the database. This feature is also useful in general when auto-commit is off as users may forget to commit ongoing transactions.

One thing I have noticed, when I add a new row and delete it immediately without saving it and try to close the query tool, the uncommitted prompt is coming.

In my opinion, it should not come, what do you think?

We should disable the prompt if auto-commit and auto-rollback both are enabled.

The uncommited prompt does not keep track of what the user has done so far, it only checks for the current transaction status. If a current transaction is ongoing, the prompt comes up. If you added a new row then deleted it without saving, the transaction status is not affected, you must have done a previous operation and had auto-commit turned off (probably the select statement).

if auto-commit & auto-rollback are both enabled then there won't be any ongoing transaction at any point, thus, the prompt will never come up.

Exactly, my point is. It should not prompt if auto-commit & auto-rollback both are enabled, but it is coming. Please see the attached video.

Agreed - this should be fixed. 

If auto-commit is turned off, it should also prompt to commit if the user hit Save in the prior step I think. Maybe reversing that prompt makes more sense in general - prompting to save rather than discard is quite normal; think about text editors etc.

 

--

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I will look into it and get back to you.

Thanks !

On Fri, Jul 12, 2019, 11:20 AM Dave Page <dpage@pgadmin.org> wrote:

On Fri, Jul 12, 2019 at 5:57 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi Yosry,

On Thu, Jul 11, 2019 at 6:50 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Hi Khushboo,

Please find an updated patch attached with the mentioned import line removed.

Looks good to me.  

On Thu, Jul 11, 2019 at 6:45 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Hi,

On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Some points I missed:

1.  I assumed that in this patch modification in case of OIDs= True (without primary key) has not considered as that is not working.

This is not implemented yet. I will work on that in a following patch soon enough.

Okay. 

2. As we are already showing the changed Data prompt on closing the Query Tool, do we really need the Uncommitted Transaction prompt? 

This is needed when auto-commit is off. Saving changes in the data grid is performed as part of the ongoing transaction (or a new one if none is ongoing). After saving the data changes the user should still commit the current transaction for the changes to be commited to the database. This feature is also useful in general when auto-commit is off as users may forget to commit ongoing transactions.

One thing I have noticed, when I add a new row and delete it immediately without saving it and try to close the query tool, the uncommitted prompt is coming.

In my opinion, it should not come, what do you think?

We should disable the prompt if auto-commit and auto-rollback both are enabled.

The uncommited prompt does not keep track of what the user has done so far, it only checks for the current transaction status. If a current transaction is ongoing, the prompt comes up. If you added a new row then deleted it without saving, the transaction status is not affected, you must have done a previous operation and had auto-commit turned off (probably the select statement).

if auto-commit & auto-rollback are both enabled then there won't be any ongoing transaction at any point, thus, the prompt will never come up.

Exactly, my point is. It should not prompt if auto-commit & auto-rollback both are enabled, but it is coming. Please see the attached video.

Agreed - this should be fixed. 

If auto-commit is turned off, it should also prompt to commit if the user hit Save in the prior step I think. Maybe reversing that prompt makes more sense in general - prompting to save rather than discard is quite normal; think about text editors etc.

 

--

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi all,

Please find attached an updated patch with the following modifications:

- Fixed the bug noticed by Khushboo, it was caused because I was using the connection status checked periodically to know the transaction status and whether a transaction is active. Monitoring the connection status could be disabled using preferences (which I assume was the case at Khushboo's). I changed the code to store the last transaction status on any query execution, polling, or saving of data changes. This transaction status is used rather than the one checked periodically as it can be disabled. I also refactored other parts of the code to make use of the stored transaction status.

- Created a new dialog that prompts the user to either commit or rollback when exiting with an active transaction. In addition, the commit button is disabled when the transaction is invalid (as the default behavior of clicking commit when the transaction is invalid is rolling back, this makes it clearer to the user that they can only rollback the transaction or cancel the exit). Preferences and documentation where updated accordingly.

- When the user performs a failed save as a part of an ongoing transaction (with auto-commit off), a notification now clarifies that only the saving action was rolledback rather than the whole transaction, therefore, there previous queries are unaffected.

Looking forward to your feedback.

Thanks !

On Fri, Jul 12, 2019 at 11:53 AM Yosry Muhammad <yosrym93@gmail.com> wrote:

I will look into it and get back to you.

Thanks !

On Fri, Jul 12, 2019, 11:20 AM Dave Page <dpage@pgadmin.org> wrote:

On Fri, Jul 12, 2019 at 5:57 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi Yosry,

On Thu, Jul 11, 2019 at 6:50 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Hi Khushboo,

Please find an updated patch attached with the mentioned import line removed.

Looks good to me.  

On Thu, Jul 11, 2019 at 6:45 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Hi,

On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Some points I missed:

1.  I assumed that in this patch modification in case of OIDs= True (without primary key) has not considered as that is not working.

This is not implemented yet. I will work on that in a following patch soon enough.

Okay. 

2. As we are already showing the changed Data prompt on closing the Query Tool, do we really need the Uncommitted Transaction prompt? 

This is needed when auto-commit is off. Saving changes in the data grid is performed as part of the ongoing transaction (or a new one if none is ongoing). After saving the data changes the user should still commit the current transaction for the changes to be commited to the database. This feature is also useful in general when auto-commit is off as users may forget to commit ongoing transactions.

One thing I have noticed, when I add a new row and delete it immediately without saving it and try to close the query tool, the uncommitted prompt is coming.

In my opinion, it should not come, what do you think?

We should disable the prompt if auto-commit and auto-rollback both are enabled.

The uncommited prompt does not keep track of what the user has done so far, it only checks for the current transaction status. If a current transaction is ongoing, the prompt comes up. If you added a new row then deleted it without saving, the transaction status is not affected, you must have done a previous operation and had auto-commit turned off (probably the select statement).

if auto-commit & auto-rollback are both enabled then there won't be any ongoing transaction at any point, thus, the prompt will never come up.

Exactly, my point is. It should not prompt if auto-commit & auto-rollback both are enabled, but it is coming. Please see the attached video.

Agreed - this should be fixed. 

If auto-commit is turned off, it should also prompt to commit if the user hit Save in the prior step I think. Maybe reversing that prompt makes more sense in general - prompting to save rather than discard is quite normal; think about text editors etc.

 

--

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--

Yosry Muhammad Yosry

Computer Engineering student,

The Faculty of Engineering,

Cairo University (2021).

Class representative of CMP 2021.

https://www.linkedin.com/in/yosrym93/