Thanks all, applied. On Wed, May 30, 2018 at 5:06 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote:
> Hello Akshay, > > Thanks for taking a look. We did the following changes over your patch: > > - Changed the XPATH to CSS_SELECTOR, please look in > pgadmin/feature_tests/query_tool_tests.py:660 and 667 > - Changed the other places in _query_tool_notify_statements to do not > use xpath. > - Moved the skip that previously was around the entire function to > surround only the pg_notify call > > Thanks > Victoria && Joao > > > On Wed, May 30, 2018 at 1:52 AM Akshay Joshi < > akshay.jo...@enterprisedb.com> wrote: > >> Hi Joao >> >> On Tue, May 29, 2018 at 9:10 PM, Joao De Almeida Pereira < >> jdealmeidapere...@pivotal.io> wrote: >> >>> Hello Akshay, >>> >>> The code looks good, we do have some minor notes for next time: >>> >>> . To make the code easier to understand we should decide on 1 term and >>> not have notifies and notifications to reference the same thing. >>> >> Fixed and used 'notifies' where it reference the same, but still at >> some places 'notifications' is used because the new tab represents the >> 'Notifications'. >> >> >>> . It would be nice if we tried not to use XPATH to get the HTML >>> components that we are testing on. Maybe try to use CSS classes. Selenium >>> also has a driver.select_by_class_name which would work well in this >>> case. >>> >> Tried to use both CSS_SELECTOR and CLASS_NAME, but it doesn't >> support string comparison(contains method). I have googled for this and >> most of the sites suggested to use XPATH where we can use contains() >> method. >> >> >>> The error that we are seeing in CI is related to pg_notify. This >>> function was introduced in 9.0 and Greenplum is still not there yet. In >>> order to solve this need to skip that part of the tests. There is an >>> example in that same file using the function _test_explain_plan_feature >>> to skip a tests depending on the Version. >>> >> OK. I have skip that part of the tests as per your suggestion. I have >> rename the method "_test_explain_plan_feature" to >> "_supported_server_version" which is more meaningful then the previous one. >> >> Attached is the modified patch. Please review it. >> >> >>> Thanks >>> Anthony && Joao >>> >>> >>> On Mon, May 28, 2018 at 1:34 AM Akshay Joshi < >>> akshay.jo...@enterprisedb.com> wrote: >>> >>>> Hi Hakers, >>>> >>>> On my last patch feature tests were failed for GreenPlum5 database only >>>> not for PostgreSQL. I have verified that on https://gpdb-dev.bosh. >>>> pivotalci.info/teams/pgadmin/pipelines/pgadmin-patch/jobs/ >>>> run-tests/builds/93. >>>> >>>> Can someone from Pivotal team help me, as I don't have GreenPlum >>>> database and don't know why it is failing. >>>> >>>> On Sat, May 26, 2018 at 5:47 PM, Akshay Joshi < >>>> akshay.jo...@enterprisedb.com> wrote: >>>> >>>>> Hi Hackers, >>>>> >>>>> As per suggestion by Dave and discussion with in the team, I have >>>>> modified the logic again. Following are the modifications: >>>>> >>>>> - Instead of waiting for another query to execute on the session >>>>> where 'LISTEN' command has been executed, we fetched the notify >>>>> messages in >>>>> the connection status polling logic. Doing this user will get the >>>>> notify >>>>> messages asap. >>>>> - Instead of showing all the notifications in single alertify >>>>> dialog, we call *alertify.info <http://alertify.info>('<msg>') *for >>>>> individual notifications. >>>>> - Created new tab 'Notifications' in Query Tool where all the >>>>> notify messages will be recorded with the timestamp and payload. >>>>> >>>>> Please review the latest patch. >>>>> >>>>> On Tue, May 22, 2018 at 2:43 PM, Dave Page <dp...@pgadmin.org> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Tue, May 22, 2018 at 10:01 AM, Akshay Joshi < >>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi Dave >>>>>>> >>>>>>> On Tue, May 22, 2018 at 2:02 PM, Dave Page <dp...@pgadmin.org> >>>>>>> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Tue, May 22, 2018 at 9:13 AM, Dave Page <dp...@pgadmin.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi >>>>>>>>> >>>>>>>>> On Tue, May 22, 2018 at 7:07 AM, Akshay Joshi < >>>>>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>>>>> >>>>>>>>>> Hi Hackers, >>>>>>>>>> >>>>>>>>>> As per suggestion by Dave, I have modified the logic and now >>>>>>>>>> notifications are popped up in alertify dialog(refer >>>>>>>>>> Notify_Messages.png) as and when received on that session where >>>>>>>>>> "LISTEN" is executed. Attached is the modified patch, please review >>>>>>>>>> it. >>>>>>>>>> >>>>>>>>>> To test this feature following steps need to perform: >>>>>>>>>> >>>>>>>>>> - Apply the patch. >>>>>>>>>> - Run pgAdmin4 >>>>>>>>>> - Connect to any database server and open query tool. >>>>>>>>>> - Execute 'LISTEN foo;' command. >>>>>>>>>> - Open another query tool window and execute 'NOTIFY foo'. >>>>>>>>>> (This is without payload). >>>>>>>>>> - Execute 'select pg_notify('foo', 'Hello')' query (with >>>>>>>>>> payload). >>>>>>>>>> - Go to the query tool window from where 'LISTEN' was >>>>>>>>>> executed and run any other query. >>>>>>>>>> >>>>>>>>>> I think there was a small misunderstanding here - I was >>>>>>>>> suggesting that each notification be displayed in an Alertify >>>>>>>>> notification, >>>>>>>>> e.g. using alertify.message('A notification of FOO was received with >>>>>>>>> payload '1234'...') >>>>>>>>> >>>>>>>> >>>>>>> If there are too many notifications then it's annoying for >>>>>>> user to popped up N number of alertify dialogs. Notification is >>>>>>> only receives when any other query execute on the session where "LISTEN" >>>>>>> command executes. So for example I have NOTIFY 10 times from different >>>>>>> sessions and execute any other query on the session("LISTEN" one), 10 >>>>>>> alertify dialog will be popped up. >>>>>>> >>>>>> >>>>>> Sure, but then a) it's the users choice to listen for something very >>>>>> noisey, and b) if there are many notifications then the message box >>>>>> dialogue will become huge. >>>>>> >>>>>> The other nice thing about using notifications is that they don't >>>>>> require any acknowledgement; they show you the event happened, and then >>>>>> get >>>>>> out of the way, allowing you to jump to the messages tab if needed. >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> And it failed tests: https://gpdb-dev.bosh. >>>>>>>> pivotalci.info/teams/pgadmin/pipelines/pgadmin-patch/jobs/ >>>>>>>> run-tests/builds/85 :-( >>>>>>>> >>>>>>>> Again it's timeout issue, not able to reproduce on my machine >>>>>>> will look into it. Maybe will have to add webDriverWait. >>>>>>> >>>>>> >>>>>> OK. >>>>>> >>>>>> >>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>> - >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Mon, May 21, 2018 at 1:36 PM, Akshay Joshi < >>>>>>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>>>>>> >>>>>>>>>>> Hi Dave >>>>>>>>>>> >>>>>>>>>>> On Fri, May 18, 2018 at 4:56 PM, Dave Page <dp...@pgadmin.org> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Fri, May 18, 2018 at 12:11 PM, Akshay Joshi < >>>>>>>>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi Dave >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, May 18, 2018 at 3:58 PM, Dave Page <dp...@pgadmin.org> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hi >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Wed, May 16, 2018 at 2:51 PM, Anthony Emengo < >>>>>>>>>>>>>> aeme...@pivotal.io> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hey, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> The code looks great! The tests all passed as well. >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Agreed - however, unless you check the Messages panel, you're >>>>>>>>>>>>>> not likely to see that a message was received. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Can we also show each message in an alertify panel? >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> We need to change the design I guess, as we are currently >>>>>>>>>>>>> send this as part of Messages. We will have to send this >>>>>>>>>>>>> separately and >>>>>>>>>>>>> show it in the alertify panel. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Yeah. Unfortunately I think notifications need to be more >>>>>>>>>>>> "active" than the messages. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I am working on the above. Can we add one preferences >>>>>>>>>>> setting to "ON/OFF" this alertify panel ? >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Dave Page >>>>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>>>> Twitter: @pgsnake >>>>>>>>>>>> >>>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> *Akshay Joshi* >>>>>>>>>>> >>>>>>>>>>> *Sr. Software Architect * >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> *Akshay Joshi* >>>>>>>>>> >>>>>>>>>> *Sr. Software Architect * >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Dave Page >>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>> Twitter: @pgsnake >>>>>>>>> >>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Dave Page >>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>> Twitter: @pgsnake >>>>>>>> >>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>> The Enterprise PostgreSQL Company >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> *Akshay Joshi* >>>>>>> >>>>>>> *Sr. Software Architect * >>>>>>> >>>>>>> >>>>>>> >>>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> *Akshay Joshi* >>>>> >>>>> *Sr. Software Architect * >>>>> >>>>> >>>>> >>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>>> >>>> >>>> >>>> >>>> -- >>>> *Akshay Joshi* >>>> >>>> *Sr. Software Architect * >>>> >>>> >>>> >>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>> >>> >> >> >> -- >> *Akshay Joshi* >> >> *Sr. Software Architect * >> >> >> >> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >> > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
