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

Hi all,
Please find attached a patch with all the changes (from the beginning of
the project). What I added in this patch:

1- Fixed #1 and #2 that Mr. Toshniwal pointed out in his code review. Still
waiting for a reply on #3.

2- When data is edited in Query Tool with auto-commit turned off, the
notification message now tells the user that they need to commit these
changes.

3- The new icon is added and the functionality of both icons are now
completely separated as follows:
a) Save File button: exclusively for saving the query file (disabled in
View Table mode).
b) Save Data Changes button: for saving changes in data grid in both
modes.
I completely separated the 2 functionalities in all related files and
modules. I also fixed an existing bug that went as follows:
- The user has unsaved edits (existed in View Table mode).
- The user tries to close the panel, they are asked if they want to
save the changes.
- If they choose to save and the save failed (null in a non-null column
for example), the panel closes anyway.
The panel now does not close if the save failed.

Something that is missing with the new button is the shortcut, I don't know
how to modify the Preferences in the configuration database. I could not
find the code responsible for adding data in the Preferences table and so.
Any help?

4- A savepoint is now created before any attempts are made to save data
changes, if the operation fails, the transaction is rolled back only till
the savepoint, keeping the previous SQL in the same transaction unharmed.
The whole transaction is rolled back if none existed in the first place.

5- Fixed a bug with all Alertiy.js confirm dialogs where line break would
break words.

6- I re-implemented the code responsible for handling the panel close event
in following way:
- The event used to handle one of two mutually exclusive events (or
neither): exiting with unsaved changes in the query or exiting with unsaved
changes in the data.
- As both can happen simultaneously now, I re-implemented this code to
check for multiple cases and produce sequential dialogs for different cases
(asynchronously to avoid freezing the page) . I also added a dialog that
asks for user confirmation when exiting with an un-commited transaction (or
data changes save).

I have several questions:
- How can I edit the data in the configuration database (specifically the
preferences), what parts of the code are responsible for this?
- For running python tests, how should I produce an appropriate
test_config.json.in file for my environment?
- After running python and feature tests, changes were made to nearly all
the files (git status shows modifications in a ton of files), is there
something I have done wrong?
- When closing a panel in pgAdmin 4, my browser keeps asking me if I want
to leave the page or stay which I think might be annoying to some users
(specially when closing several tabs at once). We already produce dialogs
if any changes are unsaved, the browsers' ones are unnecessary. Is this
produces by our code or automatically by the browser? any way around it? I
use Firefox.
- What else is missing from this patch to make it applicable ? I would like
to produce a release-ready patch if possible. If so, I can continue working
on the project on following patches, I just want to know what is the
minimum amount of work needed to make this patch release-ready (especially
that changes are being made in the master branch that require me to re-edit
parts of the code that I have written before to keep things in-sync).
- For the bug that I reported before (generated queries in Query History
appear in a distorted way for the user), to get the actual query that is
being executed I can use the mogirfy() function of psycopg2 but I need
access to a cursor. I can get one directly in save_changed_data() function
by using conn.conn.cursor() but then I would be bypassing the wrapper
Connection class. Should I modify the wrapper Connection class and add a
function that can provide a cursor (or a wrapper around cursor.mogrify() )?
Thoughts?

Here are things I think I might be working on next (share your thoughts):
- Make the transaction status more prominent.
- Handle cases where one or more columns can be made read-only for the
remaining of the resultset to be updatable (for example: SELECT col1, col2,
col1 || col2 as concat FROM some_table;). This will require modifying some
of the data that is sent from the backend to the frontend and a lot o
modifications (i think) in the front-end for handling columns individually.

Thanks everyone and sorry for the long email !

On Thu, Jun 20, 2019 at 4:54 PM Yosry Muhammad <yosrym93(at)gmail(dot)com> wrote:

>
>
> On Thu, Jun 20, 2019 at 7:49 AM Aditya Toshniwal <
> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>
>> [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:
>>
>>>
>>> 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)
>>
>>>
>>>
>>
> I shortened both function names and fixed the hard-coded color. For #1, in
> the QueryToolCommand different processing of the primary keys occur in
> is_query_resultset_updatable function, where the attribute number of the
> primary keys is compared against columns and so. The only repeated part in
> check_for_updatable_resultset_and_primary_keys is the part where pk_names
> string is created in the required format (which is only a few lines of
> code). I could factor it out to a utility function - takes primary_keys
> dict and returns the pk_names string in the required format. What do you
> think?
>
> These changes (together with other changes that I am working on) will be
> included in the next version of this patch.
>
> Thanks !
>
>
> --
> *Yosry Muhammad Yosry*
>
> Computer Engineering student,
> The Faculty of Engineering,
> Cairo University (2021).
> Class representative of CMP 2021.
> https://www.linkedin.com/in/yosrym93/
>

--
*Yosry Muhammad Yosry*

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
https://www.linkedin.com/in/yosrym93/