Re: [pgAdmin4][Patch] Feature #3204 Notify/Listen not working in version 2.1

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

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.

Thanks
Anthony && Joao
​

On Mon, May 28, 2018 at 1:34 AM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)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(dot)joshi(at)enterprisedb(dot)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 <dpage(at)pgadmin(dot)org> wrote:
>>
>>>
>>>
>>> On Tue, May 22, 2018 at 10:01 AM, Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave
>>>>
>>>> On Tue, May 22, 2018 at 2:02 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Tue, May 22, 2018 at 9:13 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Tue, May 22, 2018 at 7:07 AM, Akshay Joshi <
>>>>>> akshay(dot)joshi(at)enterprisedb(dot)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(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Dave
>>>>>>>>
>>>>>>>> On Fri, May 18, 2018 at 4:56 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Fri, May 18, 2018 at 12:11 PM, Akshay Joshi <
>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Dave
>>>>>>>>>>
>>>>>>>>>> On Fri, May 18, 2018 at 3:58 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi
>>>>>>>>>>>
>>>>>>>>>>> On Wed, May 16, 2018 at 2:51 PM, Anthony Emengo <
>>>>>>>>>>> aemengo(at)pivotal(dot)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*
>