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(at)pgadmin(dot)org> wrote:

> Hi
>
> On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad <yosrym93(at)gmail(dot)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
>