Re: [GSoC] Finalized First Patch

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