Make TopBar Public By Default

[codydaig]

In reference to #583.

[mleanos]

@codydaig I've tested this, and it works as expected.

However, I'm having an issue with the isPublic field of the Menu. I'm not quite sure what the point of this field is; Is it necessary since we have the isPublic field on individual Menu Items & Sub Items? Especially, since this change (rightfully) has moved the isPublic inheritance from the Menu, to the Items.

It doesn't make sense that I would need to set topbar isPublic: true in order to have just one Menu Item (in this case Articles) render for Guests. I'd rather explicitly define what Menu Items are public.

We'd have to modify the shouldRender() member of the Menu service to accommodate what I'm proposing.

Anyone else have any input on this? @rhutchison @trainerbill

[codydaig]

@mleanos The way I went about this implementation is that all menu items have a default isPublic value of false. However SubMenuItems inherit their parent's isPublic status still.

It doesn't make sense that I would need to set topbar isPublic: true in order to have just one Menu Item (in this case Articles) render for Guests. I'd rather explicitly define what Menu Items are public.

That is what this PR fixes. It makes the topBar public by default.

However, I'm having an issue with the isPublic field of the Menu. I'm not quite sure what the point of this field is; Is it necessary since we have the isPublic field on individual Menu Items & Sub Items? Especially, since this change (rightfully) has moved the isPublic inheritance from the Menu, to the Items.

If i'm understanding your proposal correctly: Having the isPublic option on the menu bar allows people to create navigation bars that you must be logged in for it to show at all. However, that option can be left out and it defaults to true.
https://github.com/codydaig/mean/blob/feature/issue583/modules/core/client/services/menus.client.service.js#L63

[mleanos]

@codydaig I realize that the isPublic field was intended to be used to distinguish the behavior of multiple Menu's from each other; providing configurable rendering.

What I'm trying to wrap my head around is the purpose of the isPublic field. If we're setting the roles & isPublic fields on the Menu Items & Sub Items, these settings should determine the rendering of the Menu. With this PR, the isPublic field for the Menu solely acts as an overriding setting. This is where the logic breaks down for me.

This line seems to be useless https://github.com/meanjs/mean/blob/master/modules/core/client/services/menus.client.service.js#L27 as it only serves as an override; when the settings on the Menu items should determine this.

If I don't set any of my Menu Items' roles or isPublic fields, the default is set to none & false respectively. Now, if I set the topbar Menu isPublic: false, the topbar still displays. However, it doesn't render any of the Menu items. This clearly, IMHO, demonstrates the uselessness of the isPublic field. Now, if we're considering having a separate Menu, say sidebar that we want hidden to Guests, then this should be accomplished with a different field. How it stands now, the isPublic field is being used incorrectly to serve two purposes.

My proposed change would look something like this...

// A private function for rendering decision
    var shouldRender = function (user) {
      var render = false;

      if (user) {
        if (!!~this.roles.indexOf('*')) {
          render = true;
        } else {
          for (var userRoleIndex in user.roles) {
            for (var roleIndex in this.roles) {
              if (this.roles[roleIndex] === user.roles[userRoleIndex]) {
                render = true;
              }
            }
          }
        }
      } else {

        if (this.items) {
          this.items.forEach(function (menuItem) {
            if (menuItem.items) {
              menuItem.items.forEach(function (subMenuItem) {
                if (subMenuItem.isPublic) render = true;
              });
            }
            else {
              if (menuItem.isPublic) render = true;
            }
          });
        } else {
          if (this.isPublic) render = true;
        }          
      }

      return render;
    };

If the user isn't authenticated, then we should check if there are any Menu items that do allow public viewing.

[rhutchison]

@mleanos @codydaig @lirantal I think isPublic property should be removed, and we should only rely on the roles declaration. We can have a 'guest' role which covers unauthenticated users.

[codydaig]

Sounds Good. I've got a proposed solution coming soon. Thanks for the feedback! :-D

[codydaig]

@rhutchison @mleanos I know the build will probably fail because I haven't modified the tests yet. But, I wanted to get your opinions on this strategy of removing the isPublic.

It removes the isPublic option and relies on the roles to determine if it should render or not. '*' means it will render for everyone, including users that are not logged in. And since every user gets created with the 'user' role, then you just have to add the 'user' to the list of allowed roles to make it so only people who are logged in can view it.

I'm open to suggestions, this was just the fix that came into mind first.

[mleanos]

@codydaig I know we've talked quite a bit about this on Gitter yesterday... Just wanna get this out to the rest of the team.

From my perspective, the Menu defaultRoles shouldn't be public; it should be locked down to [user, admin]. If I want to have a Menu item or sub items public, I would go in and make that change. It seems less prone to error that way; and I'd most likely have a very small % of my Menu items public.

My other consideration would be the use of the shouldRender() method on the Menu. It's needed for the Menu Items & Sub Items, but not a Menu (topbar in this case). For me the Menu is a completely different type of entity than the Items & Sub Items, but they are being treated the same.

However, removing the render check, on the Menu, in the header client view might be too big of a change right now? At least before we get a decent consensus from the community.

@lirantal @rhutchison @cdriscol @danaronoff @igorauad @lirantal @AlexisTM

[codydaig]

@mleanos I removed the tests that check for isPublic since I removed that option and solely rely on the roles option. It's ready for review! :-D

[mleanos]

@codydaig I think this looks good, and I've tested it. Everything works as expected...

I made some modifications that address the issues we've talked about. Can you review this, and see if it's in line with those concerns? If this doesn't look good enough, and we're not ready to make these changes, then I say this PR is good to go as is.

https://github.com/mleanos/mean/tree/issue583

[mleanos]

@codydaig This looks good. I've tested. Thank you for working with me on this. I think this is the right way to implement these changes.

LGTM. From my perspective, this is ready to be merged.

@rhutchison @lirantal any feedback?

[lirantal]

Good then

[anupjha]

I think there are some issues after this merge.
I could see, Sign Up & Sign In both are displaying 'Display Name' of user after logging in.
In Chat Option, even the empty messages r getting posted & 'Email' has become little blur or not readable.

[mleanos]

@anupjha I don't think this is an issue with the merge, or anything else related to the code. I've seen the issue with the Display Name showing up in place of the User's avatar. Is this what your experiencing? If so, it's most likely caused by the User's profileImageURL set to a file that it can't find. This happens when I switch between databases and/or the profile image uploads directory getting overwritten. Check that.

As far as those other issues, I can't reproduce. Can you check additional factors that might be causing issues on your end? For instance, your application by not be loading properly due to missing dependencies.

[codydaig]

@anupjha If your still having issues, go ahead and open a issue. Since this was merged in, the discussion is bound to get lost.