Re: [GSoC][Patch] Automatic Mode Detection V1

[forked the mail chain for code review]
Hi Yosry,

On Wed, Jun 19, 2019 at 5:24 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Wed, Jun 19, 2019 at 6:18 AM Yosry Muhammad <yosrym93(at)gmail(dot)com> wrote:
>
>>
>> Waiting for the icon, will set it up once it is ready.
>>
>
> It's underway :-)
>
>
>> I ran pep8 checks and JS tests on this patch, however I could not run
>> python tests due to a problem with chromedriver (working on it), please let
>> me know if any tests fail.
>>
>
> Take a look in the Makefile (or web/regression/README) and you'll see how
> you can run tests selectively - e.g. to avoid the feature tests when
> running the Python suite, you can do "python regression/runtests.py
> --exclude feature_tests"
>
> As for chromedriver, there's a utility (tools/get_chromedriver.py) you can
> use to download and install the correct version. You should save it to
> somewhere in your path; I'd suggest the bin/ directory in your virtual
> environment.
>
>
>>
>> - When revising patches, please send an updated one for the whole thing,
>>> rather than incremental ones. Incrementals are more work to apply and don't
>>> give us any benefit in return.
>>>
>>>
>> The attached patch is a single patch including all old and new increments.
>>
>
> :-)
>
> Aditya; can you do a quick code review please? Bear in mind it's a work in
> progress and there are no docs or tests etc. yet.
>
Nice work there. :)

I just had look on the code changes, and have few suggestions:
1) I found the code around primary key in the function
check_for_updatable_resultset_and_primary_keys repeating. Try if you cpuld
modify/reuse the get_primary_keys function.
2) The function name check_for_updatable_resultset_and_primary_keys could
be shorter, something like check_updatabale_rset_pkeys. Same for
__set_updatable_resultset_attributes to __set_updatable_rset_attr
3) You've used background: #f4f4f4; for .highlighted_grid_cells class. This
should go to sqleditor.scss with appropriate color from
web/pgadmin/static/scss/resources/_default.variables.scss. Hard coded color
codes are highly discouraged.
Otherwise, looks good (didn't run and check)

>
>
>>
>> - We need to add a "do you want to continue" warning before actions like
>>> Execute or EXPLAIN are run, if there are unsaved changes in the grid.
>>>
>>> - I think we should make the text in any cells that has been edited bold
>>> until saved, so the user can see where changes have been made (as they can
>>> with deleted rows).
>>>
>>
>> Both done, new rows are highlighted too.
>>
>
> Nice! I realise it's most likely not your code, but if you can fix the
> wrapping so it doesn't break mid-word, that would be good. See the attached
> screenshot to see what I mean.
>
>
>>
>>> - If I make two data edits and then delete a row, I get 3 entries in the
>>> History panel, all showing the same delete. I would actually argue that
>>> data edit queries that pgAdmin generates should not go into the History at
>>> all, but maybe they should be added albeit with a flag to say they're
>>> internal queries and an option to hide them. Thoughts?
>>>
>>
>> That was a bug with the existing 'save changes' action of 'View Data', on
>> which mine is based upon. I fixed the bug, now the queries are shown
>> correctly. However, the queries are shown in the form in which they are
>> sent from the backend to the database driver (without parameters - also an
>> already existing bug in View Data Mode), for example:
>>
>> INSERT INTO public.kweek (
>>> media_url, username, text, created_at) VALUES (
>>> %(media_url)s::character varying, %(username)s::character varying,
>>> %(text)s::text, %(created_at)s::timestamp without time zone)
>>> returning id;
>>>
>>
>> I propose two solutions:
>> 1- Hide pgadmin's generated sql from history (in both modes).
>> 2- Show the actual sql query that was executed after the parameters are
>> plugged in (more understandable and potentially helpful).
>>
>
> I like the idea of doing 2 - but I think we should have a checkbox on the
> history panel to show/hide generated queries (and we should include
> transaction control - BEGIN, COMMIT etc - in the generated query class).
>
>
>>
>>
>>> - We need to think about how data editing fits in with transaction
>>> control. Right now, it seems to happen entirely outside of it - for
>>> example, I tend to work with auto commit turned off, so my connection sits
>>> idle-in-transaction following an initial select, and remains un-affected by
>>> edits. Please think about this and suggest options for us to discuss.
>>>
>>
>> I integrated the data editing in the transaction control as you noted.
>> Now the behavior is as follows:
>> 1- In View Data mode, same existing behavior.
>> 2- In Query Tool mode:
>> - If auto-commit is on: the modifications are made and commited once save
>> is pressed.
>> - If auto-commit is off: the modifications are made as part of the
>> ongoing transaction (or a new one if no transaction is ongoing), they are
>> not commited unless the user executes a commit command (or rollback).
>>
>
> That seems to work. I think we need to make it more obvious that there's a
> transaction in progress - especially as that can be the case after the user
> hits the Save button and thinks their data is safe (a side-thought is that
> perhaps we shouldn't require the Save button to be pressed when auto-commit
> is turned off, as that's just odd). We should highlight the transaction
> state more clearly to the user, and make sure we prompt for confirmation if
> they try to close the tab or the whole window.
>
>
>> I think it makes more sense for filters to be disabled. I mean since the
>>>> user is already writing SQL it would be more convenient to just edit it
>>>> directly.
>>>>
>>>
>>> Well we're not going to just disable them - we'll either remove them, or
>>> try to make them work. I'm leaning strongly towards just removing that code
>>> entirely.
>>>
>>>
>> I meant disabling them in the query tool while keeping them in the View
>> Data mode as the user cannot edit the sql in the View Data mode. Do you
>> want to remove the feature from both modes completely?
>>
>
> I think you misunderstand - I want to remove the View Data mode entirely.
> Your work should replace it.
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
Thanks and Regards,
Aditya Toshniwal
Software Engineer | EnterpriseDB India | Pune
"Don't Complain about Heat, Plant a TREE"