Accounts dashboard base UI for Feature: "Marketplace and Shop owners should be able to create custom permission groups" #2543

rhenshaw56 · 2017-07-10T08:01:43Z

Relates to (and partly resolves) #2184 and #2194

This PR implements new layout from the designs here. It however focuses on the basic/core functionalities for Permission Groups management. More updates and styling needs to be done as marketplace work progresses.

To Test

Clone this branch, and reaction reset
Login as admin & open Accounts dashboard
In place of the old look, you should see the new default "Owner" group showing (in a Sortable table). It should show the default admin account under owner table
Click on the heading of the "Owner" table (i.e the part with text "Owner).
The ActionView should slide out. It should show 3 sections.
The first section is the "Add Admin User" section. Test it out. It should send an invitation successfully to an email filled in. Test with an existing email too. Confirm that the inline Alerts show based on success or error. Note if there are errors in the browser console too.
NB: When you send an invite, you currently won't see the account show under any group. This will be fixed in the ticket about "inviting a user directly as part of a group"
The second section is "Edit Group" section. It should show the list of current groups (plus a "New Group" item); and a Permissions list block of switches.
- The group that was selected to open the actionView should be highlighted bold. It's active permissions should also be reflected in the Permissions block
- NB: The design goal is to make the Permissions block show directly under each group, as opposed to under the entire group list. This will come in later.
- Toggling Permissions to on and off should work
- Clicking on the pencil icon in from of a group name should show a form to edit the name and description (with the current values in place)
- Clicking New Group should open similar form with blank values, and all permissions toggled off
- Test out creating new groups. Confirm it works, with no errors
- With more groups, switching a user from one group to the other (via the group dropdown in the account row) should work. (NB: Currently, you can move the default admin from owner to another group without restriction. Work will be done on this later.
The third section is to add users to a group (the currently selected group).
- If you have admin users, they'll show up in the user list here. (NB: Review how we decide admin users)
- Hovering over a user, the group name changes to action to Make the user part of the new group. Clicking this should add the user to that group

Edited by @impactmass

* Modifies Shops publication to return all shops with matching domain by removing limit * Rewrite of shop selector dropdown * Use Shops publication instead of SellerShops publication in dropdown * Remove requirement for marketplace ownership to see shop selector * Change Reaction.ShopId when shop selector is changed * TODO: Convert shop selector to React

* Eliminates console warnings

…ShopId

…ive shop

…ng added to

spencern · 2017-07-20T21:51:15Z

@mikemurray will you review the React code in this PR - I don't feel like I've got a firm enough grasp on our React patterns to do it justice?

spencern

We're almost there! Let's make sure the permissions are checking the group's shopId and see what @mikemurray thinks about the React and then I'm good to merge.

There are also some lodash functions that we don't need. I know lodash will fall back to native when available, but I'd like to remove the dependency when we can.

spencern · 2017-07-20T22:06:12Z

client/modules/accounts/templates/dashboard/dashboard.html

+    <div>
+      {{> React component=accountsDash }}
+    </div>
+    <!--<div class="container-fluid-sm">


Let's get rid of commented code

spencern · 2017-07-20T22:44:46Z

imports/plugins/core/ui/client/components/table/sortableTable.js

@@ -123,7 +126,7 @@ class SortableTable extends Component {

    // Add minWidth = undefined to override 100px default set by ReactTable
    const displayColumns = columnMetadata.map((element) => {
-      return _.assignIn({}, element, {
+      return _.extend({}, element, {


We should use native ES2015 in place of lodash when we can. Object.assign should work in this case.

Thanks, I'll change those. I inherited those changes and haven't looked at the in detail

spencern · 2017-07-20T22:47:08Z

imports/plugins/core/accounts/client/helpers/accountsHelper.js

+
+function getPermissionMap(permissions) {
+  const permissionMap = {};
+  _.each(permissions, existing => (permissionMap[existing.permission] = existing.label));


we should be able to use native Array.each here.

spencern · 2017-07-20T22:47:56Z

imports/plugins/core/ui/client/components/table/sortableTable.js


-    const pubHandle = Meteor.subscribe(publication, this.state.query, _.assignIn({}, options));
+    const pubHandle = Meteor.subscribe(publication, this.state.query, _.extend({


Object.assign should work here without needing lodash

spencern · 2017-07-20T22:50:15Z

server/methods/core/groups.js

    check(shopId, String);
-    let res;
+    let _id;

    if (!Reaction.hasPermission("admin")) {


As we chatte about, let's make sure these are checking permissions for the shopId that is associated with the group being modified.

spencern · 2017-07-20T22:50:39Z

server/methods/core/groups.js

@@ -66,7 +66,6 @@ Meteor.methods({
    check(groupId, String);
    check(newGroupData, Object);
    check(newGroupData.name, String);
-    check(newGroupData.permissions, [String]);
    check(shopId, String);

    if (!Reaction.hasPermission("admin")) {


check group shopId

spencern

Looks good. 👍 I'm curious to know why we went with _.assignIn instead of Object.assign in some of these spots, but not a blocker for sure.

impactmass · 2017-07-21T17:00:53Z

@spencern Actually, it wasn't my change. So I returned it to the original code. I can do a check to see when that component was added, and ask author if there's a specific reason it was added.

mikemurray · 2017-07-21T17:09:43Z

imports/plugins/core/accounts/client/components/accountsDashboard.js

+    if (Array.isArray(groups)) {
+      return groups.map((group, index) => {
+        return (
+          <div key={index}>


Is this <div> necessary? you can add key to the <AccountsTable />. If it necessary, put a className on it like rui accounts-table-container or something appropriate.

mikemurray · 2017-07-21T17:10:04Z

imports/plugins/core/accounts/client/components/accountsTable.js

+  getHeader(headerName) {
+    if (headerName === "name") {
+      return (
+        <div>


needs className

mikemurray · 2017-07-21T17:10:15Z

imports/plugins/core/accounts/client/components/accountsTable.js

+    }
+    if (headerName === "email") {
+      return (
+        <div>


needs className

mikemurray · 2017-07-21T17:10:25Z

imports/plugins/core/accounts/client/components/accountsTable.js

+    }
+    if (headerName === "createdAt") {
+      return (
+        <div>


needs className

mikemurray · 2017-07-21T17:12:59Z

imports/plugins/core/accounts/client/components/accountsTableCell.js

+
+    if (columnName === "button") {
+      return (
+        <div>


is this div necessary? if so needs className

mikemurray · 2017-07-21T17:13:50Z

imports/plugins/core/accounts/client/components/permissionsList.js

+
+  render() {
+    return (
+      <div>


needs className

impactmass · 2017-07-21T19:43:36Z

@mikemurray I've pushed my recent changes here, plus fixes to the few comments you added initially. You can take a look now

spencern · 2017-07-21T22:26:05Z

@mikemurray did you get a chance to look at this since updates?

mikemurray · 2017-07-21T22:31:33Z

imports/plugins/core/accounts/client/containers/editGroupContainer.js

+    this.setState({ groups, selectedGroup });
+  }
+
+  selectGroup = grp => {


Always use the paren () around the params for consistency

Fixing now.

done. we probably should add that to our eslintrc, so that it gets flagged for everyone too when working

mikemurray

looks good!

mikemurray · 2017-07-22T00:48:25Z

@spencern I leave the final task of merging up to you.

spencern · 2017-07-22T06:52:40Z

bitHound seems to be struggling, so going to merge this. If we end up with any eslint errors we'll fix them once bitHound comes through.

I ran though creating groups and moving users between groups again and it works well.

spencern and others added 30 commits June 6, 2017 12:20

Set ShopId on shop selector change

ac4494a

Merge branch 'spencer-code-cleanup' into marketplace-shop-switcher

8b3df17

* Eliminates console warnings

Regular Shop Selector should just adjust only the route.

44ed9bc

Remove empty line at top of navbar

150514f

Change Reaction.shopId to be a ReactiveVar

8947d2c

Add closeOnClick option to DropDownMenu

9cc050d

Add shop selector to admin toolbar.

6af82e8

WIP - tons of code not committed - Init switching away from getSeller…

deae717

…ShopId

Switches dashboard context as well as published packages based on act…

6db6fe1

…ive shop

Init group file

71f7073

create group

0ef424b

Test update

f82956b

Considering side effects after group change

e82fb99

multi update on groupname

b6b93ef

Updating user after group change

ee999a9

Simpleschema fix

1f55590

Resolve update when group name changes

13430c8

add combinedroles

9011584

Fix error

8d24925

Init group/addUser

f0cb22c

Updating user group array and roles

951fb0e

more error handling on functions

d997f2a

debug group/adduser

448df6f

set field to optional

005d659

Update to remove user

c547026

Update to remove user

71ecdf4

Refactoring updates on groups user belongs to

63188cb

Refactoring updates on group name

f688503

Quick template scaffold to test adding perms to group

6698d8c

check for admin permissions for the shopId of the group a user is bei…

f1b82e4

…ng added to

impactmass changed the title ~~[WIP] Accounts dashboard base UI for Feature: "Marketplace and Shop owners should be able to create custom permission groups"~~ Accounts dashboard base UI for Feature: "Marketplace and Shop owners should be able to create custom permission groups" Jul 20, 2017

spencern requested a review from mikemurray July 20, 2017 21:50

spencern suggested changes Jul 20, 2017

View reviewed changes

impactmass self-assigned this Jul 21, 2017

Remove commented code, and update permissions check

1f7f8de

spencern approved these changes Jul 21, 2017

View reviewed changes

Fix linting error by removing unused lodash import

5ae737d

mikemurray reviewed Jul 21, 2017

View reviewed changes

imports/plugins/core/accounts/client/components/permissionsList.js Outdated

render() {

return (

<div>

Copy link

Member

mikemurray Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs className

impactmass added 3 commits July 21, 2017 19:35

Refactor components and containers

6a6f495

Merge branch origin into rowland-accounts-react-conversion-2184

c87490c

Add class to divs

643b059

mikemurray reviewed Jul 21, 2017

View reviewed changes

Ensure arrow-parens on all funcs

55fdf71

mikemurray approved these changes Jul 22, 2017

View reviewed changes

spencern merged commit df0b24a into marketplace Jul 22, 2017

spencern deleted the rowland-accounts-react-conversion-2184 branch July 22, 2017 06:53

impactmass mentioned this pull request Jul 24, 2017

[Marketplace] Marketplace and Shop owners should be able to create custom permission groups #2194

Closed

spencern mentioned this pull request Oct 11, 2017

Release 1.5.0 #3089

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accounts dashboard base UI for Feature: "Marketplace and Shop owners should be able to create custom permission groups" #2543

Accounts dashboard base UI for Feature: "Marketplace and Shop owners should be able to create custom permission groups" #2543

rhenshaw56 commented Jul 10, 2017 •

edited by impactmass

Loading

spencern commented Jul 20, 2017

spencern left a comment

spencern Jul 20, 2017

spencern Jul 20, 2017

impactmass Jul 21, 2017

spencern Jul 20, 2017

spencern Jul 20, 2017

spencern Jul 20, 2017

spencern Jul 20, 2017

spencern left a comment

impactmass commented Jul 21, 2017

mikemurray Jul 21, 2017

mikemurray Jul 21, 2017

mikemurray Jul 21, 2017

mikemurray Jul 21, 2017

mikemurray Jul 21, 2017

mikemurray Jul 21, 2017

impactmass commented Jul 21, 2017

spencern commented Jul 21, 2017

mikemurray Jul 21, 2017

impactmass Jul 21, 2017

impactmass Jul 21, 2017

mikemurray left a comment

mikemurray commented Jul 22, 2017

spencern commented Jul 22, 2017


		const pubHandle = Meteor.subscribe(publication, this.state.query, _.assignIn({}, options));
		const pubHandle = Meteor.subscribe(publication, this.state.query, _.extend({

Accounts dashboard base UI for Feature: "Marketplace and Shop owners should be able to create custom permission groups" #2543

Accounts dashboard base UI for Feature: "Marketplace and Shop owners should be able to create custom permission groups" #2543

Conversation

rhenshaw56 commented Jul 10, 2017 • edited by impactmass Loading

Edited by @impactmass

spencern commented Jul 20, 2017

spencern 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

spencern left a comment

Choose a reason for hiding this comment

impactmass commented Jul 21, 2017

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

impactmass commented Jul 21, 2017

spencern commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemurray left a comment

Choose a reason for hiding this comment

mikemurray commented Jul 22, 2017

spencern commented Jul 22, 2017

rhenshaw56 commented Jul 10, 2017 •

edited by impactmass

Loading