Re: [GSoC] Query History Upgrade - Mailing list pgadmin-hackers

Hi Yosry,

On Mon, Aug 12, 2019 at 2:00 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Hi Aditya,

On Mon, Aug 12, 2019 at 9:04 AM Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:

Hi Yosry,

Nice work there. It seems to be working fine except a few suggestions:

1) Fix pep8 issues

2) DOM Statements like below can be avoided and html can be added directly to main template of $el instead of adding extra operations of find, prepend and append. Plus, it makes it difficult to understand what will the DOM look like.

this.$el.find('.query').prepend('<i id="query_source_icon" class="query-history-icon sql-icon-lg"></i>');
$container.append($toggleEl).append(self.$el);

3) Change the below to use class d-none with toggleClass('d-none') for consistency across.

    this.$el.find('.pgadmin-query-history-entry').each(function() {
      $(this).toggle();
    });

Please find an updated patch attached with the above issues fixed. The pep8 issue was in the test, I didn't re-check pep8 after writing the test - my bad. 

 

4) I may be wrong, but I'm seeing the flash icon for view/edit data queries and view table icon for query tool queries. Looks like they are swapped.

 

They seem to be in the right place for me, would you mind rechecking?

Now there are showing fine.

However, the feature test case is failing for me. I tried 2 times:

=============Running the test cases for 'PostgreSQL 11'=============
runTest (pgadmin.feature_tests.query_tool_journey_test.QueryToolJourneyTest)
Tests the path through the query tool ... Copy rows... OK.
Copy columns... OK.
History tab... OK.
Updatable resultsets...FAIL

======================================================================
FAIL: runTest (pgadmin.feature_tests.query_tool_journey_test.QueryToolJourneyTest)
Tests the path through the query tool
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py", line 79, in runTest
    self._test_updatable_resultset()
  File "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py", line 240, in _test_updatable_resultset
    self._check_query_results_editable(query, False)
  File "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py", line 378, in _check_query_results_editable
    is_editable = self._check_cell_editable(1)
  File "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py", line 395, in _check_cell_editable
    self.assertFalse('editable' in cell_classes)
AssertionError: True is not false

----------------------------------------------------------------------
Ran 1 test in 38.118s

FAILED (failures=1) 

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/

--

Thanks and Regards,

Aditya Toshniwal

Software Engineer | EnterpriseDB India | Pune

"Don't Complain about Heat, Plant a TREE"