[WIP] Add a feedback dialog box #93

siddhantwahal · 2019-06-14T14:59:01Z

This PR adds a Feedback/Bugs dialog box to apptools. This dialog box allows the client-user to quickly report bugs or request features. Messages are sent directly to the developer team's slack channel.

Files:

feedbackbot/model.py: Logic for the client-user interface.
feedbackbot/view.py: Client-user side GUI
feedbackbot/utils.py: Some helpful functions.
feedbackbot/tests: Unit tests.
examples/example.py: An example of how the developer-user, i.e., a developer who wishes to integrate this menu item in their app, would go about doing so.

To-do list:

codecov-io · 2019-06-14T15:06:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@30cd14d). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #93   +/-   ##
=========================================
  Coverage          ?   40.51%           
=========================================
  Files             ?      240           
  Lines             ?     8912           
  Branches          ?     1158           
=========================================
  Hits              ?     3611           
  Misses            ?     5155           
  Partials          ?      146

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30cd14d...d651bf8. Read the comment docs.

cfarrow · 2019-06-14T15:06:31Z

This is an intern project. I'll be reviewing and guiding the work.

corranwebster

I've given some feedback, some nit-picky some fairly important.

Other points:

this is a replacement for/improvement upon apptools.logging.agent so you need to think what should be done with that code. Either this should replace it, or you should remove that code as part of this PR (which breaks backwards compatibility, but I think that's OK). Following the naming of that package (eg. QualityAgent rather than Feedback) may be a good idea.
this PR is totally dependent on Slack and it's API, but that is just one of the possible channels that we might want to use to solicit user feedback, depending on the needs of our clients. Also there is a high likelihood that Slack's API will change or go away or cost money over the course of years and we don't want to have to re-write a quality agent a 4th (or 5th or 6th time). At a minimum we should be able to support e-mail delivery of feedback in the future; and being able to send to a generic RESTful endpoint woudl also make sense. Think about architecting this so that Slack is just one possible delivery layer, even if you don't implement anything else just yet. This would mean rather than having a send function you have an abstract base class which has an abstract send method and which can take a feedback message object; concrete subclasses will format it and send it as appropriate for that delivery mechanism. Users of your code can then implement as desired.
it would be nice to have a screenshot of the view as part of the PR.

Other than that, basically looks OK, and pretty good for a first shot at a TraitsUI interface.

corranwebster · 2019-06-14T15:11:38Z

apptools/feedback/feedbackbot/model.py

+
+    """
+
+    first_name = Str(msg_meta=True)


Please give a comment for all trait definitions, eg. #: The first name of the user.

A secondary comment is that it's probably easier to have just one name field unless there is some compelling reason to split it.

corranwebster · 2019-06-14T15:12:54Z

apptools/feedback/feedbackbot/model.py

+
+            # Send message.
+            response = client.files_upload(
+                    channels=self.channels,


Nitpick: indentation.

Please run flake8 (or equivalent) over your code.

corranwebster · 2019-06-14T15:14:50Z

apptools/feedback/feedbackbot/model.py

+
+        except slack.errors.SlackApiError as error:
+
+            print(


Don't use print, instead use logging. Standard procedure for logging in a library like this is to have at the top of the file a

import logging logger = logging.getLogger(__name__)

and then here you would use logger.error and/or logger.exception to record the error.

corranwebster · 2019-06-14T15:15:24Z

apptools/feedback/feedbackbot/model.py

+
+        except aiohttp.client_exceptions.ClientConnectorError as error:
+
+            print('Message not sent.')


Same comment here about logging.

corranwebster · 2019-06-14T15:15:39Z

apptools/feedback/feedbackbot/model.py

+
+        else:
+
+            print('Message sent successfully!')


And here, but this would be logger.info.

apptools/feedback/validating_dialog_with_feedback.py

corranwebster · 2019-06-14T15:48:19Z

apptools/feedback/feedbackbot/model.py

+    #: Main body of the feedback message.
+    description = Str(msg_meta=True)
+
+    #: The target slack channel that the bot will post to, must start with #.


"Channels" and "tokens" are not part of the concept of a "message". They are part of the concept of a (particular) delivery system and so should be factored out into their own class (see main review comment).

corranwebster · 2019-06-14T15:52:22Z

apptools/feedback/feedbackbot/model.py

+    msg = Str
+
+    #: The screenshot pixel data in raw bytes. Note that RGB[A] ordering is assumed.
+    img_bytes = Bytes


Use a numpy array (ie. image = Array(shape=(None, None, 3)) for an RGB image, (None, None, 4) for RGBA). It is easy to convert to a Pillow Image from an array, and you don't need to store auxiliary information like the height and width.

You are using Chaco and/or Enable here, so numpy is already a dependency.

apptools/feedback/feedbackbot/utils.py

apptools/feedback/feedbackbot/view.py

corranwebster · 2019-06-14T16:05:00Z

And one more thing - this PR doesn't add a feedback menu item, it provides a feedback dialog. You might want to update the title of the PR. Adding a menu item is a secondary thing and can be part of a second PR, if needed.

cfarrow

Here's a first round of comments. I'll generate more as I mess around with it.

cfarrow · 2019-06-14T15:36:23Z

apptools/feedback/feedbackbot/model.py

+    #: The screenshot height in pixels.
+    img_h = Int
+
+    @on_trait_change('+msg_meta')


For this it makes more sense to use a Property instead of a trait-change handler.

cfarrow · 2019-06-14T15:40:07Z

apptools/feedback/feedbackbot/utils.py

+plugin can be used in their application. 
+"""
+
+from PyQt4.QtGui import QPixmap


Please import from pyface.qt.QtGui so this is more portable.

cfarrow · 2019-06-14T15:42:44Z

apptools/feedback/validating_dialog_with_feedback.py

@@ -0,0 +1,468 @@
+"""


Please simplify this as much as possible and move it into examples.

It's already here: https://github.com/enthought/ets-examples/blob/master/snippets/validating_dialog.py

It shouldn't be in this at all, I think.

I meant just the menu item code. It would be good to have bare-bones example that holds the invocation sequence of the dialog.

Oh - I see it now. Yep, I agree.

siddhantwahal · 2019-06-17T14:21:51Z

@corranwebster, @cfarrow: Thanks for the feedback! I'll be pushing changes today.

cfarrow · 2019-06-21T16:12:52Z

@siddhantwahal I did not get a notification when you sent that. Sorry about that. I'll try to review soon.

siddhantwahal · 2019-06-21T16:28:55Z

@siddhantwahal I did not get a notification when you sent that. Sorry about that. I'll try to review soon.

@cfarrow No worries! I'm not totally finished making changes. I'll comment here and ping you on slack when it's ready for review.

cfarrow · 2019-06-21T16:30:29Z

Sounds good. (I got that notification. 😄)

siddhantwahal · 2019-06-24T14:30:17Z

@cfarrow I'm think I'm done with most of the big changes. The only thing remaining is a README.

I'm not sure if my approach to testing is right (I Mocked almost everything out), so any feedback there would be helpful.

cfarrow · 2019-06-24T14:31:04Z

Ok. I'll take a look today or tomorrow.

cfarrow

Things are looking good. I have another round of comments to consider.

cfarrow · 2019-06-24T21:10:06Z

apptools/feedback/examples/example.py

@@ -0,0 +1,96 @@
+"""


I think this is better placed in /examples/feedback instead of apptools/feedback/examples.

cfarrow · 2019-06-24T21:12:54Z

apptools/feedback/feedbackbot/utils.py

+    return img_array
+
+
+def initiate_feedback_dialog_(control, token, channels):


Why the trailing _? If it's for the sake of avoiding name conflict in the example, it is not necessary. (Let's talk in person if it's not clear why.)

cfarrow · 2019-06-24T21:20:08Z

apptools/feedback/feedbackbot/model.py

+    #: 3D numpy array to hold three channel (RGB) screenshot pixel data.
+    img_data = Array(shape=(None, None, 3), dtype='uint8', required=True)
+
+    # FIXME: Not sure if this the right way to go about initiating a


The right way to do this would be
compressed_img_buf = Instance(io.BytesIO, args=()).

The args= part says what to use when initializing the type (io.BytesIO).

(That said, this works. I've never seen it done this way before.)

Upon closer inspection, this is only used by the send method. It's probably best to not make it a trait and create the buffer as needed inside of send.

I moved it out assuming that it would reduce time spent in the send function. Some more experiments revealed that while time spent in send is reduced (1.57s to 1.5s), the difference is barely noticeable by the user, so now it's back in. (Note to self: time everything)

cfarrow · 2019-06-25T12:41:14Z

apptools/feedback/feedbackbot/model.py

+    token = Str(required=True)
+
+    #: The final message that gets posted to Slack.
+    msg = Property(Str, depends_on='msg_meta')


I believe this should be depends_on='+msg_meta'. It would be good to make this consistent with the syntax in the view.

cfarrow · 2019-06-25T12:54:05Z

apptools/feedback/feedbackbot/tests/test_model.py

+from apptools.feedback.feedbackbot.model import FeedbackMessage
+
+
+class TestFeedbackMessage(unittest.TestCase, UnittestTools):


These are essentially testing traits. It would be better to test the logic in the send method. This can be done by mocking slack.WebClient.files_upload and checking that the right information is passed.

cfarrow

One minor nit.

cfarrow · 2019-06-28T20:10:30Z

examples/feedback/example.py

+
+
+# View for the example app. The feedbackbot module provides a helper function
+# `initiate_feedback_dialog_` that launches the feedback dialog box. To include


Nit: this still has the trailing underscore.

apptools/feedback/feedbackbot/utils.py

apptools/feedback/feedbackbot/view.py

cfarrow · 2019-06-28T20:41:47Z

This is close to finished. There still the comment about apptools.logging.agent to consider. This is relatively simple, and should be useful as-is. I would rather not tie the completion of this that previous code. Corran, are you opposed to merging this without regard to the logging code.

cfarrow · 2019-07-24T18:54:04Z

@corran, what is your opinion about the logging stuff ?(See the comment above.)

mdickinson · 2019-10-10T14:05:53Z

@cfarrow @corran Can either of you comment on the current status of this PR? Is it still considered work-in-progress?

cfarrow · 2019-10-11T13:59:03Z

I'm in favor of merging it because it is usable as-is. There are concerns about the logging code which has a similar intent, but is constructed completely differently.

kitchoi · 2021-01-06T17:44:45Z

I expected to see changes to CI build settings as there are additional dependencies on slack here, but those changes are absent. Then I realized that the tests are actually not run on CI, probably due to the missing __init__.py files?

With that, I am not sure this can be merged as-is.

kitchoi · 2021-01-06T17:59:28Z

Once the feature is merged and released, it will be part of a published public API and refactoring will be more constrained. Just the fact a feature is live and published demands maintenance. For apptools release 5.0, we spent a lot of time discussing whether various subpackages can be removed, and when we cannot be sure, they are kept, we flake8-them, migrate tests from nose to unittest, update setup.py for their (optional) dependencies, add documentation etc. etc.

If this feature is gaining enough traction, let's make sure it is tested on CI and maybe flag the API as provisional. If there are not enough traction, we can also consider tagging the commit to persist it on the git repo instead of merging. If interest arises again, we can continue the efforts from there.

This issue may be related: #137

kitchoi · 2021-01-13T09:53:26Z

This PR needs some changes to ensure the added tests are exercised on CI.
@siddhantwahal @corranwebster @mdickinson @cfarrow Does any of you want to advocate for this PR to be fixed for it to be merged?
Does any of you want to advocate tagging this PR in the git repo for future resurrections?

mdickinson · 2021-01-14T11:54:49Z

Does any of you want to advocate for this PR to be fixed for it to be merged?

I'm not super-keen. We can fix the CI issue, but I also think there are some opportunities to rethink the design. In particular, I'd like to see the notification mechanism (Slack in this case) better separated from the dialog and screenshot stuff. It's a really interesting PR as a proof of concept, and yes, maybe worth an experimental/xxx tag, but I don't think it's ready for apptools master without a bit more work.

kitchoi · 2021-01-14T12:03:05Z

Thank you @mdickinson. Agree to what you said.

If no one wants to advocate the otherwise, I will close and tag this PR in about a week time (I will try to remember).

kitchoi · 2021-01-22T09:06:28Z

I have now tagged the last commit as experimental/featurebot referencing this PR.

Closing.

Siddhant Wahal added 4 commits June 13, 2019 14:29

Initial commit

b25c109

Don't use relative imports

faf06fc

Specifying bot OAuth token should be user-developer's concern

644b52a

Unify image interface

9dc5122

cfarrow self-requested a review June 14, 2019 15:06

corranwebster reviewed Jun 14, 2019

View reviewed changes

cfarrow reviewed Jun 14, 2019

View reviewed changes

Further simplify image interface

8e9dcc8

Siddhant Wahal added 9 commits June 17, 2019 09:39

Modify import

f2fb60b

Use single name field and use property for message

fc6e146

Fix bug in msg getter

4057d6d

Added comments

2afabb7

Added comment regarding asynchronous functionality.

b94d489

Refactor send function

b6e7b01

Better error handling and notification dialogs in controller.

82bd9dc

Refactor logic and introduce logging.

40bcab8

Improve comments

a5471da

Siddhant Wahal added 2 commits June 21, 2019 12:23

Use HasRequiredTraits

2a304a8

Add tests.

beb3813

siddhantwahal changed the title ~~[WIP] Add a feedback menu item~~ [WIP] Add a feedback dialog box Jun 21, 2019

Siddhant Wahal added 3 commits June 21, 2019 16:13

Bugfix in example

7c407d7

Use custom style for Description field

40f47fd

PEP8 compliance

b7da2c2

Put error dialog test in a helper function.

0db10aa

cfarrow reviewed Jun 25, 2019

View reviewed changes

Siddhant Wahal added 9 commits June 25, 2019 11:16

Compress image inside send function.

f60992c

Fix incorrect syntax for metadata dependence.

1a61255

Add test to ensure files_upload is called correctly

b34a768

Remove trailing _

ef1a643

Remove trailing _

1ee8f74

Move example file

d7ffe07

Add comments + fix typos

e659e49

Add README

133a35f

Fix typos in README

12691b6

cfarrow reviewed Jun 28, 2019

View reviewed changes

Fixed typo in function name

d651bf8

kitchoi closed this Jan 22, 2021


		except aiohttp.client_exceptions.ClientConnectorError as error:

		print('Message not sent.')

		return img_array


		def initiate_feedback_dialog_(control, token, channels):

		from apptools.feedback.feedbackbot.model import FeedbackMessage


		class TestFeedbackMessage(unittest.TestCase, UnittestTools):



		# View for the example app. The feedbackbot module provides a helper function
		# `initiate_feedback_dialog_` that launches the feedback dialog box. To include

[WIP] Add a feedback dialog box #93

[WIP] Add a feedback dialog box #93

Conversation

siddhantwahal commented Jun 14, 2019 • edited Loading

codecov-io commented Jun 14, 2019 • edited Loading

Codecov Report

cfarrow commented Jun 14, 2019

corranwebster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corranwebster commented Jun 14, 2019

cfarrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddhantwahal commented Jun 17, 2019

cfarrow commented Jun 21, 2019

siddhantwahal commented Jun 21, 2019 • edited Loading

cfarrow commented Jun 21, 2019

siddhantwahal commented Jun 24, 2019

cfarrow commented Jun 24, 2019

cfarrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfarrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfarrow commented Jun 28, 2019

cfarrow commented Jul 24, 2019

mdickinson commented Oct 10, 2019

cfarrow commented Oct 11, 2019

kitchoi commented Jan 6, 2021

kitchoi commented Jan 6, 2021

kitchoi commented Jan 13, 2021

mdickinson commented Jan 14, 2021

kitchoi commented Jan 14, 2021

kitchoi commented Jan 22, 2021

siddhantwahal commented Jun 14, 2019 •

edited

Loading

codecov-io commented Jun 14, 2019 •

edited

Loading

siddhantwahal commented Jun 21, 2019 •

edited

Loading