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

Поиск

Список

Период

Сортировка

От	Yosry Muhammad
Тема	Re: [GSoC][Patch] Automatic Mode Detection V1
Дата	24 июня 2019 г. 08:38:03
Msg-id	CAFSMqn87dZ_uKjk6UEwNd=wUM4NvWEc7d1Tqs5yj-MkaVK94Kw@mail.gmail.com обсуждение исходный текст
Ответ на	Re: [GSoC][Patch] Automatic Mode Detection V1 (Yosry Muhammad <yosrym93@gmail.com>)
Ответы	Re: [GSoC][Patch] Automatic Mode Detection V1 (Aditya Toshniwal <aditya.toshniwal@enterprisedb.com>)
Список	pgadmin-hackers

Дерево обсуждения

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@gmail.com> wrote:

On Thu, Jun 20, 2019 at 7:49 AM Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:
[forked the mail chain for code review]
Hi Yosry,

On Wed, Jun 19, 2019 at 5:24 PM Dave Page <dpage@pgadmin.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/