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

Hi Yosry,

On Mon, Jun 24, 2019 at 11:08 AM Yosry Muhammad <yosrym93(at)gmail(dot)com> wrote:

> 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.
>
What I meant for the color was to use existing colors defined which are
based on a theme. The best fit for your work is $color-gray-lighter. Please
use - $color-highlighted-grid-cell : $color-gray-lighter. Or may be remove
$color-highlighted-grid-cell completely and use $color-gray-lighter in your
class.

>
> 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?
>
In the sqleditor __init__.py, you'll find a call - RegisterQueryToolPreferences
(web/pgadmin/tools/sqleditor/utils/query_tool_preferences.py). Refer the
code for btn_save_file

> - For running python tests, how should I produce an appropriate
> test_config.json.in file for my environment?
>
Copy the test_config.json.in to test_config.json in the same directory. You
just need to change the server details, sample below. You can add multiple
servers to the list:
...

"server_credentials": [
{
"name": "PostgreSQL 9.4",
"comment": "PostgreSQL 9.4 Server",
"db_username": "postgres",
"host": "localhost",
"db_password": "postgres",
"db_port": 5432,
"maintenance_db": "postgres",
"sslmode": "prefer",
"tablespace_path": "",
"enabled": true,
"default_binary_paths": {
"pg": "/Library/PostgreSQL/9.4/bin/",
"ppas": "",
"gpdb": ""
}
}
],

...

> - 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?
>
What command did you use, can you share the screenshot of the files changed?

> - 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.
>
This can be disabled from Preferences -> Browser -> Display -> Confirm on
close or refresh ?. Set it to false and you'll not get the browser warning.
This is particularly helpful if you open the query tool in a new browser
tab and close the tab itself.

> - 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).
>
@Dave Page is the right person to answer this.

> - 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?
>
Could you please share the query/screenshot ? The query history just stores
the SQL text and fetches back to show in CodeMirror. No
modifications/generation of queries is done by Query History.

>
> 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/
>

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