#482 Make stars work
Merged 6 years ago by abompard. Opened 6 years ago by ryanlerch.
ryanlerch/fedora-hubs make-stars-work  into  develop

file modified
+28 -8
@@ -537,14 +537,34 @@ 

  

      @property

      def bookmarks(self):

-         # TODO -- someday make this editable/configurable.

-         return sorted(list(set([

-             assoc.hub for assoc in self.associations

-             if assoc.role == 'member' or

-             assoc.role == 'subscriber' or

-             assoc.role == 'owner' and

-             assoc.hub.name != self.username

-         ])), key=operator.attrgetter('name'))

+         bookmarks = {

+             "starred": [],

+             "memberships": [],

+             "subscriptions": [],

+         }

+         starred_hubs = self.starred_hubs

+         memberships = self.memberships

+         for assoc in self.associations:

+             if assoc.hub.name == self.username:

+                 continue

+ 

+             if assoc.role == "stargazer":

+                 bookmarks["starred"].append(assoc.hub)

+ 

+             if ((assoc.role == "member" or assoc.role == "owner")

+                     and assoc.hub not in starred_hubs):

+                 bookmarks["memberships"].append(assoc.hub)

+ 

+             if (assoc.role == "subscriber"

+                     and assoc.hub not in starred_hubs

+                     and assoc.hub not in memberships):

+                 bookmarks["subscriptions"].append(assoc.hub)

+ 

+         bookmarks = dict(

+             (key, sorted(list(set(values)), key=operator.attrgetter('name')))

+             for key, values in bookmarks.items()

+         )

+         return bookmarks

  

      @classmethod

      def by_username(cls, username):

@@ -7,9 +7,8 @@ 

  class LeftMenu extends React.Component {

  

    render() {

-     let userEntries = [];

-     if (this.props.user.logged_in) {

-       userEntries = [

+     let leftMenuEntries = [];

+       leftMenuEntries.push(

          (

            <LeftMenuEntry

              key={this.props.user.hub}
@@ -21,33 +20,85 @@ 

            <LeftMenuEntry

              key={this.props.user.stream}

              url={this.props.user.stream}

-             icon="home"

+             icon="comment"

              text="My Stream"

              />

-         ),

-         ...this.props.user.bookmarks.map((entry) => (

+         ), (

+           <LeftMenuEntry

+             key="All groups"

+             url={this.props.allGroupsUrl}

+             icon="users"

+             text="All groups"

+             />

+         )

+       );

+       if (this.props.user.starred_hubs.length > 0){

+         leftMenuEntries.push(

+           <LeftMenuHeader

+             key="Starred"

+             text="Starred"

+             />

+         );

+       }

+       leftMenuEntries.push(

+         ...this.props.user.starred_hubs.map((entry) => (

            <LeftMenuEntry

              key={entry.url}

-             icon="bookmark"

+             icon={entry.user_hub ? "user" : "users"}

              text={entry.name}

+             user_hub={entry.user_hub}

              url={entry.url}

              cssClass={entry.cssClass}

              />

-           ))

-       ];

-     }

+         ))

+       )

+       if (this.props.user.memberships.length > 0){

+         leftMenuEntries.push(

+           <LeftMenuHeader

+             key="Memberships"

+             text="Memberships"

+             />

+         );

+       }

+       leftMenuEntries.push(

+         ...this.props.user.memberships.map((entry) => (

+           <LeftMenuEntry

+             key={entry.url}

+             icon={entry.user_hub ? "user" : "users"}

+             text={entry.name}

+             user_hub={entry.user_hub}

+             url={entry.url}

+             cssClass={entry.cssClass}

+             />

+         ))

+       )

+       if (this.props.user.subscriptions.length > 0){

+         leftMenuEntries.push(

+           <LeftMenuHeader

+             key="Subscriptions"

+             text="Subscriptions"

+             />

+         );

+       }

+       leftMenuEntries.push(

+         ...this.props.user.subscriptions.map((entry) => (

+           <LeftMenuEntry

+             key={entry.url}

+             icon={entry.user_hub ? "user" : "users"}

+             text={entry.name}

+             user_hub={entry.user_hub}

+             url={entry.url}

+             cssClass={entry.cssClass}

+             />

+         ))

+       )

      return (

        <div className="LeftMenu">

          <ul

            className="nav nav-pills flex-lg-column mb-0 rounded"

            role="navigation"

            >

-           {userEntries}

-           <LeftMenuEntry

-             url={this.props.allGroupsUrl}

-             icon="users"

-             text="All groups"

-             />

+           {leftMenuEntries}

          </ul>

        </div>

      );
@@ -93,6 +144,7 @@ 

  

  LeftMenuEntry.propTypes = {

    url: PropTypes.string.isRequired,

+   user_hub: PropTypes.bool,

    icon: PropTypes.string.isRequired,

    text: PropTypes.string.isRequired,

    cssClass: PropTypes.string,
@@ -100,3 +152,17 @@ 

  LeftMenuEntry.defaultProps = {

    cssClass: null,

  }

+ 

+ class LeftMenuHeader extends React.Component {

+     render() {

+       return (

+         <li className="nav-item ml-2 mt-2 text-muted">

+             <small><strong>{this.props.text}</strong></small>

+         </li>

+       );

+     }

+   }

+ 

+ LeftMenuHeader.propTypes = {

+   text: PropTypes.string.isRequired,

+ }

file modified
+40 -10
@@ -110,8 +110,8 @@ 

  

      <!-- Left vertical nav menu -->

      <div class="col-lg-2">

+       {% if g.auth.logged_in %}

        <ul class="nav nav-pills flex-lg-column mb-3 rounded" id="vertical-navbar" role="navigation">

-         {% if g.auth.logged_in %}

          <li class="nav-item">

            <a class="nav-link {% if request.path.endswith('/' + g.user.username + '/') %}active{% endif %}"

               href="{{ url_for("hub", name=g.user.username) }}">
@@ -122,11 +122,22 @@ 

          <li class="nav-item">

            <a class="nav-link {% if request.path.endswith(url_for("stream")) %}active{% endif %}"

               href="{{ url_for("stream") }}">

-             <span><i class="fa fa-home" aria-hidden="true"></i></span>

+             <span><i class="fa fa-comment" aria-hidden="true"></i></span>

              My Stream

            </a>

          </li>

-         {% for hub in g.user.bookmarks %}

+         <li class="nav-item">

+             <a class="nav-link" href="{{ url_for("groups") }}">

+               <span><i class="fa fa-users" aria-hidden="true"></i></span>

+               All groups

+             </a>

+           </li>

+         {% if g.user.bookmarks["starred"] %}

+           <li class="nav-item ml-2 mt-2 text-muted">

+             <small><strong>Starred</strong></small>

+           </li>

+         {% endif %}

+         {% for hub in g.user.bookmarks["starred"] %}

            <li class='nav-item idle-{{hub.activity_class}}{% if request.path.endswith('/' + hub.name + '/') %} active{% endif %}'>

              <a class="nav-link" href="{{ url_for("hub", name=hub.name) }}">

                <span><i class="fa fa-bookmark" aria-hidden="true"></i></span>
@@ -134,15 +145,34 @@ 

              </a>

            </li>

          {% endfor %}

+         {% if g.user.bookmarks["memberships"] %}

+           <li class="nav-item ml-2 mt-2 text-muted">

+               <small><strong>Memberships</strong></small>

+           </li>

          {% endif %}

-         <!-- At the end of the list, tack on a link to all groups -->

-         <li class="nav-item">

-           <a class="nav-link" href="{{ url_for("groups") }}">

-             <span><i class="fa fa-users" aria-hidden="true"></i></span>

-             All groups

-           </a>

-         </li>

+         {% for hub in g.user.bookmarks["memberships"] %}

+           <li class='nav-item idle-{{hub.activity_class}}{% if request.path.endswith('/' + hub.name + '/') %} active{% endif %}'>

+             <a class="nav-link" href="{{ url_for("hub", name=hub.name) }}">

+               <span><i class="fa fa-bookmark" aria-hidden="true"></i></span>

+               {{hub.name}}

+             </a>

+           </li>

+         {% endfor %}

+         {% if g.user.bookmarks["subscriptions"] %}

+           <li class="nav-item ml-2 mt-2 text-muted">

+             <small><strong>Subscriptions</strong></small>

+           </li>

+         {% endif %}

+         {% for hub in g.user.bookmarks["subscriptions"] %}

+           <li class='nav-item idle-{{hub.activity_class}}{% if request.path.endswith('/' + hub.name + '/') %} active{% endif %}'>

+             <a class="nav-link" href="{{ url_for("hub", name=hub.name) }}">

+               <span><i class="fa fa-bookmark" aria-hidden="true"></i></span>

+               {{hub.name}}

+             </a>

+           </li>

+         {% endfor %}

        </ul>

+       {% endif %}

      </div>

  

      <div class="col-lg-10 px-0">

file modified
+55
@@ -247,3 +247,58 @@ 

          hub.unsubscribe(ralph, role="owner")

          self.assertNotIn(ralph, hub.owners)

          self.assertIn(ralph, hub.members)

+ 

+ 

+ class ModelBookmarksTest(hubs.tests.APPTest):

+ 

+     def _add_assoc(self, hubname, username, role):

+         hub = hubs.models.Hub.query.get(hubname)

+         user = hubs.models.User.query.get(username)

+         self.session.add(hubs.models.Association(

+             hub=hub, user=user, role=role))

+ 

+     def test_user_bookmarks(self):

+         """

+         test that when we add bookmarks they show up.

+         when adding bookmarks for different hubs, we should

+         see them all in the result.

+         """

+         commops = hubs.models.Hub(name="commops")

+         self.session.add(commops)

+         self._add_assoc("infra", "ralph", "stargazer")

+         self._add_assoc("i18n", "ralph", "member")

+         self._add_assoc("commops", "ralph", "subscriber")

+         ralph = hubs.models.User.query.get("ralph")

+         self.assertEquals(len(ralph.bookmarks["starred"]), 1)

+         self.assertEquals(ralph.bookmarks["starred"][0].name, "infra")

+         self.assertEquals(len(ralph.bookmarks["memberships"]), 1)

+         self.assertEquals(ralph.bookmarks["memberships"][0].name, "i18n")

+         self.assertEquals(len(ralph.bookmarks["subscriptions"]), 1)

+         self.assertEquals(ralph.bookmarks["subscriptions"][0].name, "commops")

+ 

+     def test_user_bookmarks_star(self):

+         """

+         test that when when adding associations for the same hub,

+         we should just see the stargazer one.

+         """

+         self._add_assoc("infra", "ralph", "stargazer")

+         self._add_assoc("infra", "ralph", "member")

+         self._add_assoc("infra", "ralph", "subscriber")

+         ralph = hubs.models.User.query.get("ralph")

+         self.assertEquals(len(ralph.bookmarks["starred"]), 1)

+         self.assertEquals(ralph.bookmarks["starred"][0].name, "infra")

+         self.assertEquals(len(ralph.bookmarks["memberships"]), 0)

+         self.assertEquals(len(ralph.bookmarks["subscriptions"]), 0)

+ 

+     def test_user_bookmarks_memberships(self):

+         """

+         test that when when adding associations for the same hub,

+         we should just see the memberships one.

+         """

+         self._add_assoc("infra", "ralph", "member")

+         self._add_assoc("infra", "ralph", "subscriber")

+         ralph = hubs.models.User.query.get("ralph")

+         self.assertEquals(len(ralph.bookmarks["starred"]), 0)

+         self.assertEquals(len(ralph.bookmarks["memberships"]), 1)

+         self.assertEquals(ralph.bookmarks["memberships"][0].name, "infra")

+         self.assertEquals(len(ralph.bookmarks["subscriptions"]), 0)

file modified
+24 -3
@@ -65,13 +65,34 @@ 

          user = flask.g.user

          current_user["hub"] = flask.url_for("hub", name=user.username)

          current_user["stream"] = flask.url_for("stream")

-         current_user["bookmarks"] = []

-         for hub in user.bookmarks:

-             current_user["bookmarks"].append({

+ 

+         current_user["memberships"] = []

+         for hub in user.bookmarks["memberships"]:

+             current_user["memberships"].append({

+                 "name": hub.name,

+                 "user_hub": hub.user_hub,

+                 "url": flask.url_for("hub", name=hub.name),

+                 "cssClass": "idle-{}".format(hub.activity_class),

+             })

+ 

+         current_user["starred_hubs"] = []

+         for hub in user.bookmarks["starred"]:

+             current_user["starred_hubs"].append({

                  "name": hub.name,

+                 "user_hub": hub.user_hub,

                  "url": flask.url_for("hub", name=hub.name),

                  "cssClass": "idle-{}".format(hub.activity_class),

              })

+ 

+         current_user["subscriptions"] = []

+         for hub in user.bookmarks["subscriptions"]:

+             current_user["subscriptions"].append({

+                 "name": hub.name,

+                 "user_hub": hub.user_hub,

+                 "url": flask.url_for("hub", name=hub.name),

+                 "cssClass": "idle-{}".format(hub.activity_class),

+             })

+ 

      return current_user

  

  

This makes the stars add to the side bookmarks toolbar.
Also, did some tweaking of how the bookmarks bar is
generated and displayed. (in react only)
Fixes #475

Note that the sidebar is also implemented in flask/jinja templates, but this is currently only used in the allgroups page. That will be fixed when the allgroups page is implemented properly.

I think it would be more efficient to change the bookmarks property to return a dict keyed on the relationship type (star, membership, subscription) with the hub lists as values. Otherwise the filtering is making additional calls to the other properties which end up in SQL queries.

The userEntries name does not seem very appropriate anymore.

Please add the key="All groups" value so React can identify the item in the list (there's an error message in the JS console if you don't).

It's not actually required (see the "All groups" entry), please remove the .isRequired.

Are you sure you want to use the "home" icon here?

Thanks for the review, @abompard !

  • removed the bookmarks_starred, and other properties, and made the bookmarks property return a dict keyed to the type. Doing this broke the allgroups page (which i am planning to hack on next), so i just updated the menu there so it works the same way -- futher formatting on that page will come in a future PR
  • renamed userEntries to leftMenuEntries
  • added the key="All groups"
  • removed the is_required on the hubs_type bool
  • changed the home icon for all users back to the 'users' one, we are using this for the groups down below, but this is easily changed in the future.

rebased onto 8c4347b63a91248a621b11376b0ea2e15028faae

6 years ago

Hmm this is still calling self.starred_hubs multiple times. Actually I think it would be better to run through the self.associations list and distribute items in the resulting dict's lists. This way you'd iterate over them only once. Something like:

        bookmarks = {
            "starred": [],
            "memberships": [],
            "subscriptions": [],
        }
        for assoc in self.associations:
            if assoc.hub.name == self.username:
                continue
            if assoc.role == "stargazer":
                bookmarks["starred"].append(assoc.hub)
            elif assoc.role == "member" or assoc.role == "owner":
                bookmarks["memberships"].append(assoc.hub)
            elif assoc.role == "subscriber":
                bookmarks["subscriptions"].append(assoc.hub)
        bookmarks = dict(
            (key, sorted(list(set(values)), key=operator.attrgetter('name')))
            for key, values in bookmarks.items()
        )
        return bookmarks

(I haven't tested it)

Also, please add unit tests for that, to make sure starred hubs, member hubs and subscribed hubs don't appear multiple times for example.

rebased onto 72b4ac5f2c4ed9892e0a738511d867875f8d9b9d

6 years ago

rebased onto 7a7d879068f24c3be637f26717f5ad902cdf145c

6 years ago

That's a loooot of spaces. Are those tabulations?

This should be designteam too.

Gah, sorry -- really shouldnt update commits at midnight :)

I got the tests running, then ran lint and one of those lines with designteam in it was too long, so i changed it to design, but forgot to change it up further. Just changed it all to commops now, sorry designteam ;)

also fixed the weird indenting.

rebased onto 387bec16dcf2149db533bc7f988bb3e8076f09d9

6 years ago

You don't have to set the defaultProps here because the text property was defined as isRequired above. So there's no need for a default value, it will have to be explicitely given.

rebased onto 13dede8

6 years ago

Pull-Request has been merged by abompard

6 years ago