[forked the mail chain for code review] Hi Yosry, On Wed, Jun 19, 2019 at 5:24 PM Dave Page <dp...@pgadmin.org> wrote:
> Hi > > On Wed, Jun 19, 2019 at 6:18 AM Yosry Muhammad <yosry...@gmail.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"
