Hi Joao On Tue, May 29, 2018 at 9:10 PM, Joao De Almeida Pereira <jdealmeidapereira @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*
RM_3204_v6.patch
Description: Binary data
