PR#458: implement hubs widget as per the mockup - fedora-hubs

fedora-hubs

#458 implement hubs widget as per the mockup

Merged 6 years ago by abompard. Opened 6 years ago by ryanlerch.

ryanlerch/fedora-hubs my-hubs into develop

implement my hubs widget as per the mockup

Ryan Lerch • 6 years ago

e766f53

Rename Subscriptions widget to 'My Hubs'

Ryan Lerch • 6 years ago

d788a54

hubs/default_config.py

file modified

+1 -1

		`@@ -65,10 +65,10 @@`
		`'hubs.widgets.irc:IRC',`
		`'hubs.widgets.meetings:Meetings',`
		`'hubs.widgets.memberships:Memberships',`
		`+ 'hubs.widgets.my_hubs:MyHubs',`
		`'hubs.widgets.pagure_pr:PagurePRs',`
		`'hubs.widgets.pagureissues:PagureIssues',`
		`'hubs.widgets.rules:Rules',`
		`'hubs.widgets.sticky:Sticky',`
		`- 'hubs.widgets.subscriptions:Subscriptions',`
		`'hubs.widgets.workflow.updates2stable:Updates2Stable',`
		`]`

hubs/defaults.py

file modified

+1 -1

		`@@ -40,7 +40,7 @@`
		`}))`
		`hub.widgets.append(widget)`
		`widget = hubs.models.Widget(`
		`- plugin='subscriptions', index=3,`
		`+ plugin='my_hubs', index=3,`
		`_config=json.dumps({`
		`'username': username,`
		`}))`

hubs/static/css/style.css

file modified

+22

		`@@ -390,3 +390,25 @@`
		`.small {`
		`font-size: small;`
		`}`
		`+`
		`+ .monogram-avatar{`
		`+ width:64px;`
		`+ height:64px;`
		`+ font-size:54px;`
		`+ line-height: 64px;`
		`+ font-weight: 700;`
		`+ text-align: center;`
		`+ }`
		`+`
		`+ /Move these color defs into fedora-bootstrap/`
		`+ .bg-fedora-blue{background-color:#3c6eb4;}`
		`+ .bg-fedora-magenta{background-color: #db3279;}`
		`+ .bg-fedora-orange{background-color: #e59728;}`
		`+ .bg-fedora-green{background-color: #79db32;}`
		`+ .bg-fedora-purple{background-color: #a07cbc;}`
		`+ .text-fedora-blue-dark{color: #294a7a;}`
		`+ .text-fedora-magenta-dark{color: #9a1b51;}`
		`+ .text-fedora-orange-dark{color: #9a6213;}`
		`+ .text-fedora-green-dark{color: #488b18;}`
		`+ .text-fedora-purple-dark{color: #70488f;}`
		`+`

hubs/tests/widgets/test_my_hubs.py

file added

+35

		`@@ -0,0 +1,35 @@`
		`+ from __future__ import unicode_literals`
		`+`
		`+ from . import WidgetTest`
		`+ import hubs.models # noqa: E402`
		`+`
		`+`
		`+ class MyHubsTest(WidgetTest):`
		`+`
		`+ plugin = "my_hubs"`
		`+`
		`+ def populate(self):`
		`+ super(MyHubsTest, self).populate()`
		`+ self._add_widget_under_test()`
		`+ hub = hubs.models.Hub.by_name('infra')`
		`+ hub.subscribe(hubs.models.User.by_username('ralph'), 'owner')`
		`+`
		`+ hub = hubs.models.Hub.by_name('i18n')`
		`+ hub.subscribe(hubs.models.User.by_username('ralph'), 'subscriber')`
		`+`
		`+ self.session.add(hubs.models.Hub(name="designteam"))`
		`+ hub = hubs.models.Hub.by_name('designteam')`
		`+ hub.subscribe(hubs.models.User.by_username('ralph'), 'member')`
		`+`
		`+ def test_myhubs_output(self):`
		`+ self._update_widget_config({`
		`+ "username": "ralph"`
		`+ })`
		`+ func = self.widget.module.get_cached_functions()['GetSubs']`
		`+ result = func(self.widget).execute()`
		`+ self.assertEquals(len(result['memberships']), 1)`
		`+ self.assertEquals(len(result['ownerships']), 1)`
		`+ self.assertEquals(len(result['subscriptions']), 1)`
		`+ self.assertEquals(result['memberships'][0].name, "designteam")`
		`+ self.assertEquals(result['ownerships'][0].name, "infra")`
		`+ self.assertEquals(result['subscriptions'][0].name, "i18n")`

hubs/widgets/my_hubs/__init__.py

file added

+65

		`@@ -0,0 +1,65 @@`
		`+ from __future__ import unicode_literals`
		`+`
		`+ import hashlib`
		`+`
		`+ import hubs.models`
		`+ from hubs.utils import get_fedmsg_config`
		`+ from hubs.widgets.base import Widget`
		`+ from hubs.widgets.view import RootWidgetView`
		`+`
		`+ fedmsg_config = get_fedmsg_config()`
		`+`
		`+`
		`+ class MyHubs(Widget):`
		`+`
		`+ name = "my_hubs"`
		`+ position = "right"`
		`+ label = "My Hubs"`
		`+ hidden_if_empty = True`
		`+ hub_types = ['user']`
		`+`
		`+`
		`+ class BaseView(RootWidgetView):`
		`+`
		`+ def get_context(self, instance, args, *kwargs):`
		`+ # since this widget only shows on user hubs, we assume`
		`+ # hub.name == the username`
		`+ username = instance.hub.name`
		`+`
		`+ user = hubs.models.User.by_username(username)`
		`+ monograms = {}`
		`+ ownerships = []`
		`+ assoc = []`
		`+ for hub in user.ownerships:`
		`+ if not hub.user_hub:`
		`+ ownerships.append(hub)`
		`+ assoc.append(hub.name)`
		`+ monograms[hub.name] = generate_monogram(hub.name)`
		`+ memberships = []`
		`+ for hub in user.memberships:`
		`+ if not hub.user_hub:`
		`+ if hub.name not in assoc:`
		`+ memberships.append(hub)`
		`+ assoc.append(hub.name)`
		`+ monograms[hub.name] = generate_monogram(hub.name)`
		`+ subscriptions = []`
		`+ for hub in user.subscriptions:`
		`+ if not hub.user_hub:`
		`+ if hub.name not in assoc:`
		`+ subscriptions.append(hub)`
		`+ monograms[hub.name] = generate_monogram(hub.name)`
		`+ return dict(`
		`+ ownerships=ownerships,`
		`+ memberships=memberships,`
		`+ subscriptions=subscriptions,`
		`+ monograms=monograms,`
		`+ )`
		`+`
		`+`
		`+ def generate_monogram(hubname):`
		`+ colours = ["blue", "green", "magenta", "orange", "purple"]`
		`+ colour_index = int(hashlib.md5(hubname.encode('utf8')).hexdigest(), 16) % 5`
		`+ return ("<div class='monogram-avatar bg-fedora-%s text-fedora-%s-dark'>"`
		`+ "%s</div>") % (colours[colour_index],`
		`+ colours[colour_index],`
		`+ hubname[0].upper())`

hubs/widgets/my_hubs/templates/root.html

file added

+42

		`@@ -0,0 +1,42 @@`
		`+ <div class="p-2">`
		`+ <ul class="list-unstyled">`
		`+ {% for ownership in ownerships %}`
		`+ <li class="media py-2">`
		`+ <div class="align-self-center mr-3">`
		`+ {{monograms[ownership.name]}}`
		`+ </div>`
		`+ <div class="media-body">`
		`+ <div>`
		`+ <strong><a href="{{url_for('hub', name=ownership.name)}}">{{ownership.name}}</a></strong>`
		`+ </div>`
		`+ <small>Group Administrator</small>`
		`+ </div>`
		`+ </li>`
		`+ {% endfor %}`
		`+ {% for membership in memberships %}`
		`+ <li class="media py-2">`
		`+ <div class="align-self-center mr-3">`
		`+ {{monograms[membership.name]}}`
		`+ </div>`
		`+ <div class="media-body">`
		`+ <div>`
		`+ <strong><a href="{{url_for('hub', name=membership.name)}}">{{membership.name}}</a></strong>`
		`+ </div>`
		`+ <small>Member</small>`
		`+ </div>`
		`+ </li>`
		`+ {% endfor %}`
		`+ {% for subscription in subscriptions %}`
		`+ <li class="media py-2">`
		`+ <div class="align-self-center mr-3">`
		`+ {{monograms[subscription.name]}}`
		`+ </div>`
		`+ <div class="media-body">`
		`+ <div>`
		`+ <strong><a href="{{url_for('hub', name=subscription.name)}}">{{subscription.name}}</a></strong>`
		`+ </div>`
		`+ <small>Subscribed</small>`
		`+ </div>`
		`+ </li>`
		`+ {% endfor %}`
		`+ </div>`

hubs/widgets/subscriptions/__init__.py

file removed

-79

		`@@ -1,79 +0,0 @@`
		`- from __future__ import unicode_literals`
		`-`
		`- import flask`
		`- from fedmsg.meta import msg2usernames`
		`- from fedmsg.meta.base import BaseConglomerator as BC`
		`-`
		`- import hubs.models`
		`- from hubs.utils import get_fedmsg_config`
		`- from hubs.widgets import validators`
		`- from hubs.widgets.base import Widget`
		`- from hubs.widgets.view import RootWidgetView`
		`- from hubs.widgets.caching import CachedFunction`
		`-`
		`-`
		`- fedmsg_config = get_fedmsg_config()`
		`-`
		`-`
		`- class Subscriptions(Widget):`
		`-`
		`- name = "subscriptions"`
		`- position = "right"`
		`- display_title = "Hubs"`
		`- hidden_if_empty = True`
		`- parameters = [`
		`- dict(`
		`- name="username",`
		`- label="Username",`
		`- default=None,`
		`- validator=validators.Username,`
		`- help="A FAS username.",`
		`- )]`
		`- hub_types = ['user']`
		`-`
		`-`
		`- class BaseView(RootWidgetView):`
		`-`
		`- def get_context(self, instance, args, *kwargs):`
		`- context = {}`
		`- get_subs = GetSubs(instance)`
		`- context.update(get_subs())`
		`- return context`
		`-`
		`-`
		`- # function hyperlinks the hubs in the subscription widget`
		`- def manage_subscriptions(items):`
		`- return [`
		`- '<a href="{link}">{item}</a>'.format(`
		`- link=flask.url_for('hub', name=item),`
		`- item=item,`
		`- ) for index, item in enumerate(items[0:3])`
		`- ]`
		`-`
		`-`
		`- class GetSubs(CachedFunction):`
		`-`
		`- def execute(self):`
		`- username = self.instance.config["username"]`
		`- user = hubs.models.User.by_username(username)`
		`- ownerships = [u.name for u in user.ownerships]`
		`- memberships = [u.name for u in user.memberships]`
		`- subscriptions = [u.name for u in user.subscriptions]`
		`- subscriptions_list = manage_subscriptions(subscriptions)`
		`- memberships_list = manage_subscriptions(memberships)`
		`- return dict(`
		`- associations=memberships + ownerships,`
		`- ownerships=ownerships,`
		`- memberships=memberships,`
		`- subscriptions=subscriptions,`
		`- ownerships_text=BC.list_to_series(ownerships),`
		`- memberships_text=BC.list_to_series(memberships_list),`
		`- subscriptions_text=BC.list_to_series(subscriptions_list),`
		`- )`
		`-`
		`- def should_invalidate(self, message):`
		`- if not message['topic'].endswith('hubs.associate'):`
		`- return False`
		`- username = self.instance.config['username']`
		`- users = msg2usernames(message, **fedmsg_config)`
		`- return username in users`

hubs/widgets/subscriptions/templates/root.html

file removed

-15

		`@@ -1,15 +0,0 @@`
		`- {% if associations %}`
		`-`
		`- {% if memberships %}`
		`- <p><strong>Belongs to: </strong> {{memberships_text}}</p>`
		`- {% endif %}`
		`- <hr/>`
		`- {% if subscriptions %}`
		`- <p><strong>Subscribes to:</strong>`
		`- {% for subscription in subscriptions %}`
		`- <a href="{{ url_for('hub', name=subscription)}}">`
		`- {{subscription}}</a>`
		`- {% endfor %}</p>`
		`- {% endif %}`
		`-`
		`- {% endif %}`

ryanlerch commented 6 years ago

implement my hubs widget as per the mockup

a rework of the hubs widget to match the mockup, including a
rename from "subscriptions" to "my_hubs"

fixes #436

abompard commented 6 years ago

Thanks! Do you think some unit tests on the view function would be useful? Maybe a couple to make sure nothing gets mixed up between the different hub types and association types?

ryanlerch commented 6 years ago

Okies, added a test to test it out, also fixed a couple of other tests that were failing (i still had subscriptions in the default widget list, rather than the renamed my_hubs) -- whoops!

2 new commits added

implement my hubs widget as per the mockup
Rename Subscriptions widget to 'My Hubs'

6 years ago

2 new commits added

implement my hubs widget as per the mockup
Rename Subscriptions widget to 'My Hubs'

6 years ago

rebased onto 536605ef5ee35419f90df8ed333f0cc35a0d6fb9

6 years ago

abompard commented on line 31 of hubs/tests/widgets/test_my_hubs.py 6 years ago

Leftover print statement! :-)

abompard commented 6 years ago

When I run it on python3 I get the following traceback:

[...]
  File "/home/abompard/Fedora/hubs/fedora-hubs/hubs/widgets/my_hubs/__init__.py", line 64, in execute
    monograms[hub.name] = generate_monogram(hub.name)
  File "/home/abompard/Fedora/hubs/fedora-hubs/hubs/widgets/my_hubs/__init__.py", line 45, in generate_monogram
    colour_index = int(hashlib.md5(hubname).hexdigest(), 16) % 5
TypeError: Unicode-objects must be encoded before hashing

Please encode the hubname to utf-8 before passing it to md5().

Also, do we want to have the username as a config value, or should we just use the hub name? It seems strange to want to display someone else's memberships on one's hub.

Edited 6 years ago by abompard

ryanlerch commented 6 years ago

Okies, removed the debug print statement i left in there.

Also now encoding the hubname before hashing it so it works on python3.

Also, there was a failing test after rebasing the wigdgets only on certain hub types PR, due to renaming this hub from suscriptions to my_hubs, so i fixed that one too. I squashed these into the main commit here, will do the config value change in a seperate commit.

2 new commits added

implement my hubs widget as per the mockup
Rename Subscriptions widget to 'My Hubs'

6 years ago

1 new commit added

remove username config value from my_hubs widget

6 years ago

ryanlerch commented 6 years ago

okies, added the extra commit that removes the username config item, and just uses the hubname.

rebased onto 2de99eca3ddee4bb306c0c0bb788f9d388a803be

6 years ago

abompard commented 6 years ago

I get these errors when running tox -e lint:

./hubs/widgets/my_hubs/__init__.py:8:1: F401 'hubs.widgets.validators' imported but unused
from hubs.widgets import validators
^
./hubs/widgets/my_hubs/__init__.py:48:35: W291 trailing whitespace
        # hub.name == the username 
                                  ^
./hubs/widgets/my_hubs/__init__.py:53:9: E741 ambiguous variable name 'l'
        l = []
        ^

Also, there's a small conflict in a test due to a recent commit, please rebase.

rebased onto fea8c58766f690937c4dcb74ab43635c7cf36b1a

6 years ago

3 new commits added

remove username config value from my_hubs widget
implement my hubs widget as per the mockup
Rename Subscriptions widget to 'My Hubs'

6 years ago

2 new commits added

implement my hubs widget as per the mockup
Rename Subscriptions widget to 'My Hubs'

6 years ago

2 new commits added

implement my hubs widget as per the mockup
Rename Subscriptions widget to 'My Hubs'

6 years ago

ryanlerch commented 6 years ago

@abompard rebased, and fixed the lint issues!

abompard commented 6 years ago

Thanks, it all looks good except it does not update when I join a hub. The invalidation method is waiting for a "hubs.associate" message that is never sent. We have two options:

have Hubs send that message, but since Hubs does not currently send any fedmsgs, I'm afraid this might mean some in-depth work to teach it to do that.
get rid of the cached function and do it all in the view.

Since all the cached function does is to do SQL queries, I'd be in favor of option 2 at least for the MVP. We will very probably need to teach Hubs to send fedmsgs in the future though.

ryanlerch commented 6 years ago

if we do it all in the view, it still wont update async, right?

FWIW, that invalidation stuff was there to begin with too -- i thought it was not working because it was not hooked up to fedmsg proper.

Edited 6 years ago by ryanlerch

abompard commented 6 years ago

Yeah there's a lot of not working stuff that was there to begin with ;-)
If we do it in the view the SQL queries will be done on each widget display. It's not ideal from a performance point of view, of course, but every widgets are displayed async anyway so it won't slow down the loading of the page.

ryanlerch commented 6 years ago

okies! removed the caching stuff from this one, and updated the commit.

rebased onto d788a54

6 years ago