Postgres Pro
Downloads
Home   >   mailing lists

Thread: [GSoC] Finalized First Patch

[GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
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.
Attachment

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
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@gmail.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.


--
Yosry Muhammad Yosry

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
Attachment

Re: [GSoC] Finalized First Patch

From
Dave Page
Date:
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@gmail.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@gmail.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.


--
Yosry Muhammad Yosry

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.


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

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

Re: [GSoC] Finalized First Patch

From
Dave Page
Date:
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
Attachment

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
Sounds good ! 

Looking forward to any feedback.

Thanks.

On Fri, Jul 5, 2019, 1:01 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

Re: [GSoC] Finalized First Patch

From
Khushboo Vashi
Date:


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.

Sure. I am on it. 
Thanks! 

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

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

Re: [GSoC] Finalized First Patch

From
Khushboo Vashi
Date:
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

Re: [GSoC] Finalized First Patch

From
Khushboo Vashi
Date:
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.
2. As we are already showing the changed Data prompt on closing the Query Tool, do we really need the Uncommitted Transaction prompt? 

On Wed, Jul 10, 2019 at 12:03 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
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

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
Hi Khushboo,

On Wed, Jul 10, 2019, 8:33 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi Yosry,

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

Thanks ! I am doing my best.

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?

I will look into those and get back to you asap. Thanks for the feedback !

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
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.

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.

Thanks and Regards :D

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
Hi,
Please find an updated patch attached.

On Wed, Jul 10, 2019 at 8:33 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
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.

I tried to reproduce the issue but it works fine for me. Please make sure that you edit the empty cell (to add a new row) and press enter for the edits on the grid to take effect.
 
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.

It was just a sanity check, just in case someone calls the function incorrectly. I removed it and added comments below the function header indicating when it should be called.
 
3. In the Preferences, the label of the keyboard shortcut "Save Data Changes" should be "Save data changes".

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

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


I don't know which part exactly are you referring to.
In the previous patch, I have already updated keyboard_shortcuts.rst to document the new button shortcut, in addition to preferences.rst to document the new 'Alert on uncommited changes' option. I also updated query_tool.rst, query_tool_toolbar.rst & editgrid.rst to document the new features. Which part is missing?
 
Looking forward to your feedback and comments.
Thanks !
Attachment

Re: [GSoC] Finalized First Patch

From
Khushboo Vashi
Date:
Hi Yosry,

On Thu, Jul 11, 2019 at 4:10 AM Yosry Muhammad <yosrym93@gmail.com> wrote:
Hi,
Please find an updated patch attached.

On Wed, Jul 10, 2019 at 8:33 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
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.

I tried to reproduce the issue but it works fine for me. Please make sure that you edit the empty cell (to add a new row) and press enter for the edits on the grid to take effect.
 
Right, on the enter, the button gets enabled, not on the focus out, so this is by design, not something your patch caused.
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.

It was just a sanity check, just in case someone calls the function incorrectly. I removed it and added comments below the function header indicating when it should be called.
Please make sure to remove "from pgadmin.tools.sqleditor.utils.constant_definition import ASYNC_OK" line from the file as not required anymore. 
 
3. In the Preferences, the label of the keyboard shortcut "Save Data Changes" should be "Save data changes".

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

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


I don't know which part exactly are you referring to.
In the previous patch, I have already updated keyboard_shortcuts.rst to document the new button shortcut, in addition to preferences.rst to document the new 'Alert on uncommited changes' option. I also updated query_tool.rst, query_tool_toolbar.rst & editgrid.rst to document the new features. Which part is missing?
 
My bad, I have applied the patch on the web folder, so the difference of rst files didn't get applied. Now, I can see the doc updates and it looks good to me.

Looking forward to your feedback and comments.
Thanks !

Thanks,
Khushboo 

Re: [GSoC] Finalized First Patch

From
Khushboo Vashi
Date:
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.

Thanks,
Khushboo
 
Thanks and Regards :D

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
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.

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
I missed attaching the updated patch, here it is !


On Thu, Jul 11, 2019 at 3:20 PM Yosry Muhammad <yosrym93@gmail.com> wrote:
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.


--
Yosry Muhammad Yosry

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
Attachment

Re: [GSoC] Finalized First Patch

From
Dave Page
Date:


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

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
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

Re: [GSoC] Finalized First Patch

From
Yosry Muhammad
Date:
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.
Attachment

Re: [GSoC] Finalized First Patch

From
Dave Page
Date:
Patch committed (with some trivial editorial work on the docs). 

Congratulations - that's impressive work!

On Tue, Jul 16, 2019 at 6:03 AM Yosry Muhammad <yosrym93@gmail.com> wrote:
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.


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

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