fedora-hubs

#501 Add issues widget
Merged by abompard. Opened by shaily.
shaily/fedora-hubs issues  into  develop

Download 501.patch

Related: #1 and #391

1 new commit added

  • Issues: Add links to view more

8 new commits added

  • Issues: Add links to view more
  • Issues: Add tests and fix resulting errors
  • Issues: Invalidate cache on fedmsg
  • Issues: Change parameter type to string
  • Issues: Rename details to VCS
  • Issues: Use react widget
  • Issues: Fetch VCS details from hub config
  • Issues: Initial commit

Hub Config - https://i.imgur.com/inHSGY3.png
Widget Config - https://i.imgur.com/rdzNGyc.png
Widget - https://i.imgur.com/lyGKzB3.png

8 new commits added

  • Issues: Add links to view more
  • Issues: Add tests and fix resulting errors
  • Issues: Invalidate cache on fedmsg
  • Issues: Change parameter type to string
  • Issues: Rename details to VCS
  • Issues: Use react widget
  • Issues: Fetch VCS details from hub config
  • Issues: Initial commit

8 new commits added

  • Issues: Add links to view more
  • Issues: Add tests and fix resulting errors
  • Issues: Invalidate cache on fedmsg
  • Issues: Change parameter type to string
  • Issues: Rename details to VCS
  • Issues: Use react widget
  • Issues: Fetch VCS details from hub config
  • Issues: Initial commit

Sorry for the redundant messages, I was amending a few commits.
I could definitely use reviews on the React code, this is my first time.
(Specifically on whether to break any of the stuff down into more components - say an Issue component)

Also, since Github doesn't emit any fedmsgs, how should we decide when to invalidate its cache (setting an expiration time of say 2 minutes) or whether to not cache that data at all?
Edited by shaily

1 new commit added

  • Issues: Update bugzilla endpoint

1 new commit added

  • Issues: Fix bug in sorting

1 new commit added

  • Issues: Fix bug in bugzilla endpoint

1 new commit added

  • Issues: Move views to separate file

rebased onto 79f36ceab0fd047cdf130c7bab68468550bfb6b8

rebased onto 8673df484cb82e17915910a6dcb26d8ecfb37c0e

rebased onto 79f36ceab0fd047cdf130c7bab68468550bfb6b8

rebased onto 8673df484cb82e17915910a6dcb26d8ecfb37c0e

rebased onto 79f36ceab0fd047cdf130c7bab68468550bfb6b8

rebased onto 07bea7ada5b17469ff5b8ca8624d755f6a830f5c

1 new commit added

  • Issues: Handle exception in PagureUser validator

1 new commit added

  • Issues: Handle exception in PagureUser validator

1 new commit added

  • Issues: Handle missing hub config

14 new commits added

  • Issues: Handle missing hub config
  • Issues: Handle exception in PagureUser validator
  • Issues: Move views to separate file
  • Issues: Fix bug in bugzilla endpoint
  • Issues: Fix bug in sorting
  • Issues: Update bugzilla endpoint
  • Issues: Add links to view more
  • Issues: Add tests and fix resulting errors
  • Issues: Invalidate cache on fedmsg
  • Issues: Change parameter type to string
  • Issues: Rename details to VCS
  • Issues: Use react widget
  • Issues: Fetch VCS details from hub config
  • Issues: Initial commit

1 new commit added

  • Issues: Show widget only on user hubs

Please name variables in a more explicit manner, so that a reader won't have to go back to the function arguments to see what x is. I suggest configParam for example, but anything works as long as it makes it clear what the variable is about.

Also, you don't actually need i apparently, you could just use the config parameter as a key.

I think it would be safer to record e.target.value in a variable before using it in setConfig(), because in React event instances are re-used (for performance reasons). So e.target.value may change down the road when async kicks in.

Do you actually need injectIntl ? you don't seem to use the intl property anywhere, and the FormattedMessage components don't require the injectIntl HOC.

You could rewrite this logic as a if (...) {...} else if (...) {...} else {...} block, that would save some indentation and brain processing for the reader (which is more important! :-) )

In general I recommend the "Line of sight" practice when writing code: https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88

This is a very big block which would be more readable in its own sub-component.

When you have a big block of output that is generated in a loop like here, it's in general a sign that a sub-component would be useful (and make the code more clear).

I would encourage you to do this kind of actions in React and not rely on jQuery. You could replicate this with a onClick handler that would toggle a boolean in the sate, and the "more" section would only be displayed when the state variable is true.

In this case, it will stop working if you have more than one issues widget on the page, because the data-target attrbute points to an HTML id attribute, and there can only be one element with the same HTML id on a page.

This footer is pretty big and could have its own component. On top of that it would separate the footer state (used for toggling, see comment below) from the widget state (that would be passed down as props to the footer component).

This could be done in Javascript since it's basically presentation and does not use information on the server that the JS does not have (it only uses the mode variable).

Actually, we do have a hook to send fedmsgs on Github changes:
http://fedora-fedmsg.readthedocs.io/en/latest/topics.html#github

It would be nice to be able to set this 15 limit in the widget parameters.

I made some comments in the code. In general, this is a very good quality PR, thanks @shaily for your efforts.

1 new commit added

  • Issues: Refactoring after review

1 new commit added

  • Issues: Rename config parameter

2 new commits added

  • Issues: Set title in React
  • Issues: Fix bug in cache invalidation

rebased onto 66b689c9543b8fd3ebb173b74b7c6b49b903e360

rebased onto e46644d01a81adc4d610f51b26fba70da09b3fe9

rebased onto fcf3eb43abc0781bc942b2f222be2ef782de1399

rebased onto 52e6c911545f4698dc3253bea6d6f620b3819bfe

19 new commits added

  • Issues: Set title in React
  • Issues: Fix bug in cache invalidation
  • Issues: Rename config parameter
  • Issues: Refactoring after review
  • Issues: Show widget only on user hubs
  • Issues: Handle missing hub config
  • Issues: Handle exception in PagureUser validator
  • Issues: Move views to separate file
  • Issues: Fix bug in bugzilla endpoint
  • Issues: Fix bug in sorting
  • Issues: Update bugzilla endpoint
  • Issues: Add links to view more
  • Issues: Add tests and fix resulting errors
  • Issues: Invalidate cache on fedmsg
  • Issues: Change parameter type to string
  • Issues: Rename details to VCS
  • Issues: Use react widget
  • Issues: Fetch VCS details from hub config
  • Issues: Initial commit

If github_username is not set, its value here is None which will cause the Github API to return a failure a couple lines below.
I think both problems should be handled: return an empty list of issues when the username is not set, and display an error if the github API returns an error.
FYI the message I'm getting from Github is:

{'message': 'Validation Failed', 'documentation_url': 'https://developer.github.com/v3/search/', 'errors': [{'code': 'invalid', 'message': 'The listed users cannot be searched either because the users do not exist or you do not have permission to view the users.', 'field': 'q', 'resource': 'Search'}]}

Oh, and I see you're not calling the function if the VCS username is not set, but the cached function will be called nonetheless by the backend when a matching fedmsg arrives. So you need to handle that case in the function anyway.

Please return False explicitely at the end (for the other cases). Otherwise it will return None, and even if it might work, that's not the method's signature.
This is valid for the other VCS types too.

It would make sense for stream hubs too.

Unfortunately, if you want to handle the "stream hub" case, you can't just get self.instance.hub.config because the stream hub does not have that info, only the user hub does. Until I figure out a better way (probably by mirroring part of a user hub config into the stream hub config) you'd have to make a special case for the stream hub, do a DB lookup for the corresponding user hub and use that config to get the usernames.

This API has changed a bit, now we don't use another function, we just react to the new hubs.hub.updated fedmsg topic. You can check out how the repositories widget does it.
This comment is valid for the other functions too.

The display of the VCS links in the footer seems strange: I would expect the "view more" to be centered and to open a dropdown with the 3 VCS links. At the moment they don't have enough width to display :

screenshot
https://lut.im/lCGtoSJbRx/rMliYOFCDZvts9Vo.png

Also, please handle the case when there are no open issues : a message should be displayed.

rebased onto 83dcd78cc27ac5aabf7143c7f7ff3dd01b5cabe1

1 new commit added

  • Issues: Update cache invalidation

Well, not exactly : if it's a hub.updated message, but not about this instance, it will not reach the final else branch. I would just put the return False outside of the branch, as a final action.

2 new commits added

  • Issues: Change footer design
  • Issues: Never return None in cache invalidation

1 new commit added

  • Issues: Add to stream hubs

1 new commit added

  • Issues: Update cache invalidation

2 new commits added

  • Issues: Change No open issues message color
  • Issues: Add message on no open issues

rebased onto d87787a80436dbad2febba8af3bfa295a58e9e65

I have tested the widget and found a couple issues. To explain what they are, I'll use a diff of what solution I would suggest, but you're free to address them in another manner if you prefer.

First issue: the configuration panel for VCS does not show validation errors if there are any. I used the MailingList panel as an example of how to show validation errors in the fields, and I came up with this patch:

diff --git a/hubs/static/client/app/components/HubConfig/HubConfigPanelVCS.js b/hubs/static/client/app/components/HubConfig/HubConfigPanelVCS.js
index fb7d1478..b4916c26 100644
--- a/hubs/static/client/app/components/HubConfig/HubConfigPanelVCS.js
+++ b/hubs/static/client/app/components/HubConfig/HubConfigPanelVCS.js
@@ -59,6 +59,10 @@ export default class VCSPanel extends React.Component {
         <FormattedMessage {...messages.title} tagName="h3" />
         <FormattedMessage {...messages.intro} tagName="p" />
         {["pagure_username", "github_username", "bugzilla_email"].map(function(configParam) {
+          let invalid = null;
+          if (this.props.error && this.props.error.fields) {
+            invalid = this.props.error.fields[configParam];
+          }
           return (
             <div key={configParam} className="form-group row">
               <label htmlFor={"hub-settings-vcs-" + configParam}>
@@ -67,11 +71,16 @@ export default class VCSPanel extends React.Component {
               <input
                 type="text"
                 name={configParam}
-                className={"form-control"}
+                className={"form-control" + (invalid ? " is-invalid" : "")}
                 id={"hub-settings-vcs-" + configParam}
                 onChange={this.handleChange}
                 value={this.props.hubConfig[configParam] || ""}
                 />
+              { invalid &&
+                <div className="invalid-feedback w-100">
+                  {invalid}
+                </div>
+              }
               <p className="form-text text-muted">
                 <FormattedMessage {...messages[configParam + "_help"]} />
               </p>

It basically detects errors returned by the backend and sets the appropriate classes and feedback field.

Next issue : the widget does not update on a backend change (notified via SSE). The reason is double : the current version does not react to the needsUpdate property being set, and it uses the PureComponent class instead of the Component class, which causes it to ignore redraws on internal state changes (check out the React docs on that one).
Here's my proposed solution:

diff --git a/hubs/static/client/app/widgets/issues/Widget.js b/hubs/static/client/app/widgets/issues/Widget.js
index 889d7490..98bc88b0 100644
--- a/hubs/static/client/app/widgets/issues/Widget.js
+++ b/hubs/static/client/app/widgets/issues/Widget.js
@@ -1,5 +1,9 @@
 import React from 'react';
 import { apiCall } from '../../core/utils';
+import {
+  widgetDidUpdate,
+  widgetWillUpdate
+  } from "../../core/actions/widget";
 import WidgetChrome from '../../components/WidgetChrome';
 import PropTypes from "prop-types";
 import Spinner from "../../components/Spinner";
@@ -7,7 +11,7 @@ import Issue from "./Issue";
 import Footer from "./Footer";
-export default class IssuesWidget extends React.PureComponent {
+export default class IssuesWidget extends React.Component {
   constructor(props) {
     super(props);
@@ -26,11 +30,26 @@ export default class IssuesWidget extends React.PureComponent {
     }
   }
+  componentDidUpdate(prevProps, prevState) {
+    if (this.props.needsUpdate && !this.props.editMode && !this.state.isLoading) {
+      this.loadFromServer();
+    }
+  }
+
   loadFromServer() {
+    this.props.dispatch(widgetWillUpdate(this.props.widget.idx));
     this.setState({isLoading: true});
     apiCall(this.props.widget.urls.data).then(
       (data) => {
-        this.setState({issues: data.issues, counts: data.counts, isLoading: false});
+        this.setState({
+          issues: data.issues,
+          counts: data.counts,
+          isLoading: false,
+        },
+          () => (
+            this.props.dispatch(widgetDidUpdate(this.props.widget.idx))
+          )
+        );
       },
       (error) => {
         this.setState({error: error.message, isLoading: false});

Changes are:
- using Component,
- calling loadFromServer() when the needsUpdate property is set
- dispatching the widgetWillUpdate and widgetDidUpdate actions to inform Redux of the changes, which will reset the needsUpdate property when the update is done.

Next issue: the cached function was not invalidated when a VCS username is removed. The reason is that the check on the hubs.hub.updated message topic was done after the test on username is None. In this patch I have fixed another issue: on stream hubs the config would be properly pulled from the user hub, but since the hubs.hub.updated test was only checking the current stream hub id, it would never invalidate when the user hub is updated.
Here's my proposed solution:

diff --git a/hubs/widgets/issues/functions.py b/hubs/widgets/issues/functions.py
index e29caa80..aaa1987f 100644
--- a/hubs/widgets/issues/functions.py
+++ b/hubs/widgets/issues/functions.py
@@ -39,19 +39,19 @@ class GetBugzillaBugs(CachedFunction):
     def should_invalidate(self, message):
         if self.instance.hub.hub_type == "stream":
-            user_hub = Hub.query.filter_by(name=self.instance.hub.name).first()
-            email = user_hub.config.get('bugzilla_email')
+            watched_hub = Hub.query.filter_by(name=self.instance.hub.name).first()
         else:
-            email = self.instance.hub.config.get('bugzilla_email')
+            watched_hub = self.instance.hub
+        if message["topic"].endswith('.hubs.hub.updated'):
+            return (message['msg']['hub_id'] == watched_hub.id and
+                    'bugzilla_email' in message['msg']['changed_keys'])
+        email = watched_hub.config.get('bugzilla_email')
         if email is None:
             return False
         mode = self.instance.config['mode']
         if message['topic'].endswith('bugzilla.bug.new') or \
            message['topic'].endswith('bugzilla.bug.update'):
             return message['msg']['bug']['creator' if mode == 'author' else 'assigned_to'] == email
-        elif message["topic"].endswith('.hubs.hub.updated'):
-            return message['msg']['hub_id'] == self.instance.hub.id and \
-                   'bugzilla_email' in message['msg']['changed_keys']
         else:
             return False
@@ -88,10 +88,13 @@ class GetPagureIssues(CachedFunction):
     def should_invalidate(self, message):
         mode = self.instance.config['mode']
         if self.instance.hub.hub_type == "stream":
-            user_hub = Hub.query.filter_by(name=self.instance.hub.name).first()
-            username = user_hub.config.get('pagure_username')
+            watched_hub = Hub.query.filter_by(name=self.instance.hub.name).first()
         else:
-            username = self.instance.hub.config.get('pagure_username')
+            watched_hub = self.instance.hub
+        if message["topic"].endswith('.hubs.hub.updated'):
+            return (message['msg']['hub_id'] == watched_hub.id and
+                    'pagure_username' in message['msg']['changed_keys'])
+        username = watched_hub.config.get('pagure_username')
         if username is None:
             return False
         if message['topic'].endswith('pagure.issue.new') or \
@@ -101,9 +104,6 @@ class GetPagureIssues(CachedFunction):
            message['topic'].endswith('pagure.issue.drop'):
             entry = message['msg']['issue']['user' if mode == 'author' else 'assignee']
             return entry and entry['name'] == username
-        elif message["topic"].endswith('.hubs.hub.updated'):
-            return message['msg']['hub_id'] == self.instance.hub.id and \
-                   'pagure_username' in message['msg']['changed_keys']
         else:
             return False
@@ -139,10 +139,13 @@ class GetGithubIssues(CachedFunction):
     def should_invalidate(self, message):
         mode = self.instance.config['mode']
         if self.instance.hub.hub_type == "stream":
-            user_hub = Hub.query.filter_by(name=self.instance.hub.name).first()
-            username = user_hub.config.get('github_username')
+            watched_hub = Hub.query.filter_by(name=self.instance.hub.name).first()
         else:
-            username = self.instance.hub.config.get('github_username')
+            watched_hub = self.instance.hub
+        if message["topic"].endswith('.hubs.hub.updated'):
+            return (message['msg']['hub_id'] == watched_hub.id and
+                    'github_username' in message['msg']['changed_keys'])
+        username = watched_hub.config.get('github_username')
         if username is None:
             return False
         if message['topic'].endswith('github.issue.opened') or \
@@ -151,8 +154,5 @@ class GetGithubIssues(CachedFunction):
            message['topic'].endswith('github.issue.closed'):
             entry = message['msg']['issue']['user' if mode == 'author' else 'assignee']
             return entry and entry['login'] == username
-        elif message["topic"].endswith('.hubs.hub.updated'):
-            return message['msg']['hub_id'] == self.instance.hub.id and \
-                   'github_username' in message['msg']['changed_keys']
         else:
             return False

It moves the check for the hubs.hub.updated up in the method, and it uses a watched_hub variable to abstract out the "stream" use case.

Please note that I also just have fixed a bug in the main API views code that would have prevented a config value from being deleted, so you need to rebase if you want to test that use case.

We're almost there! Thanks for persisting. :100:

One last thing: when you're done, run the tox -e lint command and add another commit to fix the errors that flake8 has found. It's mostly about line lengths, etc. If it points out errors that you don't know how to handle properly, feel free to ask me.

rebased onto 5c4d7bd0c2b28654c2f3c6dbd3dc1db14453c6e2

4 new commits added

  • Issues: Fix lint warnings
  • Issues: Update CachedFunctions
  • Issues: Update widget on backend change
  • Issues: Show validation errors in hub config

Thanks a lot for reviewing!

A small request, could you please verify if the cache invalidation is functioning correctly on widget / hub config change?
I think there's some problem on my setup.
I verified that the appropriate widget.updated message is being published, but I can't reproduce the correct functioning of the triage queue picking it up and passing on a widget-cache type task to the worker.
The issue doesn't appear to be in the way the CachedFunction is implemented.
Edited by shaily

1 new commit added

  • Issues: Refactoring views.py

I think I found why (or at least a potential reason). When updating the widget state after calling the backend, you don't clear the error state variable. So if you went back to having no VCS configured, you can't ever go to the issues display again unless you reload the page.
This is a possible patch:

--- a/hubs/static/client/app/widgets/issues/Widget.js
+++ b/hubs/static/client/app/widgets/issues/Widget.js
@@ -45,6 +45,7 @@ export default class IssuesWidget extends React.Component {
           issues: data.issues,
           counts: data.counts,
           isLoading: false,
+          error: null,
         },
           () => (
             this.props.dispatch(widgetDidUpdate(this.props.widget.idx))

rebased onto c66b212b29b1028efbf9d1eb9655aa82fdf3a163

rebased onto 8c911650e5909464113e1ad9eb468c9f6c3cb555

Excellent, thanks a lot @shaily :100:

Pull-Request has been merged by abompard
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.