Re: [GSoC] Finalized First Patch
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.va...@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 wrote: > >> Hi >> >> On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad 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
Hi Khushboo, On Wed, Jul 10, 2019, 8:33 AM Khushboo Vashi < khushboo.va...@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
Hi, On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi < khushboo.va...@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
[pgAdmin4][Patch]: RM 4452 Add RE-SQL tests for Languages
Hi Hackers, Attached is the patch to fix RM #4452 "Add RE-SQL tests for Languages". Please review it. -- *Thanks & Regards* *Akshay Joshi* *Sr. Software Architect* *EnterpriseDB Software India Private Limited* *Mobile: +91 976-788-8246* RM_4452.patch Description: Binary data
pgAdmin 4 commit: Add documentation on using Traefik with pgAdmin mount
Add documentation on using Traefik with pgAdmin mounted under a subdirectory, and tidy up the reverse proxying docs a little. Branch -- master Details --- https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=49503dc73d4f8eb4859683d63fca2542888073ca Modified Files -- docs/en_US/container_deployment.rst | 54 - 1 file changed, 47 insertions(+), 7 deletions(-)
pgAdmin 4 commit: Added re_sql test cases for privileges in Foreign Dat
Added re_sql test cases for privileges in Foreign Data Wrappers
Branch
--
master
Details
---
https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=6c7e95a4638e2db631a20626f1565ace2e60c0cd
Modified Files
--
.../foreign_data_wrappers/sql/9.3_plus/acl.sql | 1 +
.../foreign_data_wrappers/sql/default/acl.sql | 1 +
.../tests/default/alter_fdw_rename.sql | 13 --
.../{ => pg}/9.3_plus/alter_fdw_change_opt2.sql| 0
.../tests/{ => pg}/9.3_plus/alter_fdw_comment.sql | 0
.../tests/{ => pg}/9.3_plus/alter_fdw_opt1.sql | 0
.../tests/{ => pg}/9.3_plus/alter_fdw_opt2.sql | 0
.../tests/pg/9.3_plus/alter_fdw_privileges.sql | 16 +
.../tests/{ => pg}/9.3_plus/alter_fdw_rename.sql | 0
.../{ => pg}/9.3_plus/alter_fdw_validator.sql | 0
.../tests/{ => pg}/9.3_plus/create_fdw.sql | 0
.../tests/{ => pg}/9.3_plus/test.json | 28 ++
.../9.3_plus}/alter_fdw_change_opt2.sql| 0
.../9.3_plus}/alter_fdw_comment.sql| 0
.../{default => ppas/9.3_plus}/alter_fdw_opt1.sql | 0
.../{default => ppas/9.3_plus}/alter_fdw_opt2.sql | 0
.../tests/ppas/9.3_plus/alter_fdw_privileges.sql | 16 +
.../tests/ppas/9.3_plus/alter_fdw_rename.sql | 13 ++
.../9.3_plus}/alter_fdw_validator.sql | 0
.../{default => ppas/9.3_plus}/create_fdw.sql | 0
.../tests/{default => ppas/9.3_plus}/test.json | 28 ++
21 files changed, 103 insertions(+), 13 deletions(-)
RE-SQL tests patch for packages node
Hi Dave, I have attached the patch for RE-SQL test cases for *Packages* node. Thanks! -- *Regards,* *Navnath Gadakh* re_sql_packages_tests_v1.patch Description: Binary data
Re: [GSoC] Finalized First Patch
Hi Yosry, On Thu, Jul 11, 2019 at 4:10 AM Yosry Muhammad wrote: > Hi, > Please find an updated patch attached. > > On Wed, Jul 10, 2019 at 8:33 AM Khushboo Vashi < > khushboo.va...@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
Hi, On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad wrote: > Hi, > > On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi < > khushboo.va...@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 >
