From a8176d3075a6da377fb3a30a365625202816ad87 Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Wed, 16 Nov 2016 16:55:18 +0100 Subject: [PATCH 1/9] refs #317: hide review buttons when user is not an editor. --- src/plugins/signoff/components.js | 23 +++++++++++++++++++++-- src/reducers/session.js | 1 - src/types.js | 3 ++- test/reducers/sessions_test.js | 1 - 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/plugins/signoff/components.js b/src/plugins/signoff/components.js index a137ed283..c2f2c6c0c 100644 --- a/src/plugins/signoff/components.js +++ b/src/plugins/signoff/components.js @@ -19,6 +19,25 @@ import AdminLink from "../../components/AdminLink"; import { ProgressBar, ProgressStep } from "./ProgressBar.js"; +function isEditor(sessionState, bucketState) { + if (typeof sessionState.serverInfo.user !== "object") { + return false; + } + const {serverInfo: {user, capabilities}} = sessionState; + if (user == null) { + return false; + } + const {signer={}} = capabilities; + const {editors_group: groupName} = signer; + const {id: userId} = user; + const {groups} = bucketState; + const editorGroup = groups.find(g => g.id === groupName); + if (!editorGroup) { + return false; + } + return editorGroup.members.includes(userId); +} + export default class SignoffToolBar extends React.Component { props: { sessionState: SessionState, @@ -67,13 +86,13 @@ export default class SignoffToolBar extends React.Component { { redirectURL: null, serverInfo: { capabilities: {}, - user: {}, }, }); }); From 7168d51056ddc8a7989f011ded018e1c6ee677e0 Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Wed, 16 Nov 2016 18:07:42 +0100 Subject: [PATCH 2/9] Render conditionnaly buttons for reviewers. --- src/plugins/signoff/components.js | 23 ++++++++++++++++------- src/plugins/signoff/types.js | 2 ++ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/plugins/signoff/components.js b/src/plugins/signoff/components.js index c2f2c6c0c..bbc15d50e 100644 --- a/src/plugins/signoff/components.js +++ b/src/plugins/signoff/components.js @@ -19,8 +19,8 @@ import AdminLink from "../../components/AdminLink"; import { ProgressBar, ProgressStep } from "./ProgressBar.js"; -function isEditor(sessionState, bucketState) { - if (typeof sessionState.serverInfo.user !== "object") { +function isMember(groupkey, source, sessionState, bucketState) { + if (sessionState.serverInfo.user == null) { return false; } const {serverInfo: {user, capabilities}} = sessionState; @@ -28,16 +28,25 @@ function isEditor(sessionState, bucketState) { return false; } const {signer={}} = capabilities; - const {editors_group: groupName} = signer; + const {[groupkey]: defaultGroupName} = signer; + const {[groupkey]: groupName=defaultGroupName} = source; const {id: userId} = user; const {groups} = bucketState; const editorGroup = groups.find(g => g.id === groupName); - if (!editorGroup) { + if (editorGroup == null) { return false; } return editorGroup.members.includes(userId); } +function isEditor(source, sessionState, bucketState) { + return isMember("editors_group", source, sessionState, bucketState); +} + +function isReviewer(source, sessionState, bucketState) { + return isMember("reviewers_group", source, sessionState, bucketState); +} + export default class SignoffToolBar extends React.Component { props: { sessionState: SessionState, @@ -86,13 +95,13 @@ export default class SignoffToolBar extends React.Component { diff --git a/src/plugins/signoff/types.js b/src/plugins/signoff/types.js index 53e4ba0f7..9d182d955 100644 --- a/src/plugins/signoff/types.js +++ b/src/plugins/signoff/types.js @@ -17,6 +17,8 @@ export type SourceInfo = { lastEditor: string, lastReviewer: string, lastStatusChanged: number, + editors_group?: string, + reviewers_group?: string, }; export type PreviewInfo = { From 99b9a470ad3a7b4ed8354495ca42ca8cdd9b9b49 Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Thu, 17 Nov 2016 11:24:53 +0100 Subject: [PATCH 3/9] Fix unmerged groups when updating route info from signoff plugin. --- src/reducers/bucket.js | 2 +- test/reducers/bucket_test.js | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/reducers/bucket.js b/src/reducers/bucket.js index 850ec6f95..4318c3220 100644 --- a/src/reducers/bucket.js +++ b/src/reducers/bucket.js @@ -48,7 +48,7 @@ export function bucket(state: BucketState = INITIAL_STATE, action: Object) { return {...INITIAL_STATE, busy: true}; } case ROUTE_LOAD_SUCCESS: { - const {bucket, groups} = action; + const {bucket, groups=state.groups} = action; return load(state, bucket, groups); } case ROUTE_LOAD_FAILURE: { diff --git a/test/reducers/bucket_test.js b/test/reducers/bucket_test.js index 6c36751ef..251d3fa2b 100644 --- a/test/reducers/bucket_test.js +++ b/test/reducers/bucket_test.js @@ -46,6 +46,13 @@ describe("bucket reducer", () => { .eql(initial); }); + it("should preserve state when no groups are passed", () => { + const initial = bucket(undefined, {type: null}); + + expect(bucket({groups: []}, {type: ROUTE_LOAD_SUCCESS, bucket: {}})) + .to.have.property("groups").eql([]); + }); + it("should update state when a bucket is passed", () => { const state = bucket(undefined, { type: ROUTE_LOAD_SUCCESS, From 7752bc21237df2a3e72774f969744474e1b94e0f Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Thu, 17 Nov 2016 11:29:01 +0100 Subject: [PATCH 4/9] WiP --- src/plugins/signoff/components.js | 27 ++++++++++++++++++++------- src/plugins/signoff/sagas.js | 12 ++++++++---- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/plugins/signoff/components.js b/src/plugins/signoff/components.js index bbc15d50e..528b03962 100644 --- a/src/plugins/signoff/components.js +++ b/src/plugins/signoff/components.js @@ -20,21 +20,25 @@ import { ProgressBar, ProgressStep } from "./ProgressBar.js"; function isMember(groupkey, source, sessionState, bucketState) { - if (sessionState.serverInfo.user == null) { - return false; - } - const {serverInfo: {user, capabilities}} = sessionState; - if (user == null) { + const {serverInfo: {user={}, capabilities}} = sessionState; + if (!user.id) { return false; } const {signer={}} = capabilities; const {[groupkey]: defaultGroupName} = signer; const {[groupkey]: groupName=defaultGroupName} = source; const {id: userId} = user; + console.log(bucketState); const {groups} = bucketState; const editorGroup = groups.find(g => g.id === groupName); if (editorGroup == null) { - return false; + // XXX for now if we can't access the group it's probably because the user + // doesn't have the permission to read it, so we mark the user has a member + // of the group. + // Later when https://github.com/Kinto/kinto/pull/891/files lands, we'll + // have access to the principals attached to a given authentication, so we'll + // be able to properly check for membership. + return true; } return editorGroup.members.includes(userId); } @@ -47,6 +51,12 @@ function isReviewer(source, sessionState, bucketState) { return isMember("reviewers_group", source, sessionState, bucketState); } +function isLastEditor(source, sessionState) { + const {serverInfo: {user={}}} = sessionState; + const {lastEditor} = source; + return user.id === lastEditor; +} + export default class SignoffToolBar extends React.Component { props: { sessionState: SessionState, @@ -87,6 +97,9 @@ export default class SignoffToolBar extends React.Component { } const {source, preview, destination} = resource; + const canReview = canEdit && + isReviewer(source, sessionState, bucketState) && + !isLastEditor(source, sessionState); // Default status is request review const step = status == "to-review" ? 1 : status == "signed" ? 2 : 0; @@ -101,7 +114,7 @@ export default class SignoffToolBar extends React.Component { { + return coll.getData(); + }) + .then(attributes => ({data: attributes})); } From b8c2e6198ad7da550b58bdfa8b2396bf7e5c000c Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Thu, 17 Nov 2016 12:17:12 +0100 Subject: [PATCH 5/9] Revert "Fix unmerged groups when updating route info from signoff plugin." This reverts commit 99b9a470ad3a7b4ed8354495ca42ca8cdd9b9b49. --- src/reducers/bucket.js | 2 +- test/reducers/bucket_test.js | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/reducers/bucket.js b/src/reducers/bucket.js index 4318c3220..850ec6f95 100644 --- a/src/reducers/bucket.js +++ b/src/reducers/bucket.js @@ -48,7 +48,7 @@ export function bucket(state: BucketState = INITIAL_STATE, action: Object) { return {...INITIAL_STATE, busy: true}; } case ROUTE_LOAD_SUCCESS: { - const {bucket, groups=state.groups} = action; + const {bucket, groups} = action; return load(state, bucket, groups); } case ROUTE_LOAD_FAILURE: { diff --git a/test/reducers/bucket_test.js b/test/reducers/bucket_test.js index 251d3fa2b..6c36751ef 100644 --- a/test/reducers/bucket_test.js +++ b/test/reducers/bucket_test.js @@ -46,13 +46,6 @@ describe("bucket reducer", () => { .eql(initial); }); - it("should preserve state when no groups are passed", () => { - const initial = bucket(undefined, {type: null}); - - expect(bucket({groups: []}, {type: ROUTE_LOAD_SUCCESS, bucket: {}})) - .to.have.property("groups").eql([]); - }); - it("should update state when a bucket is passed", () => { const state = bucket(undefined, { type: ROUTE_LOAD_SUCCESS, From f32c6ad7fee4346c612ccd22b347afda55a1ace8 Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Thu, 17 Nov 2016 16:56:16 +0100 Subject: [PATCH 6/9] Add tests for signoff plugin sagas. --- src/plugins/signoff/sagas.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/plugins/signoff/sagas.js b/src/plugins/signoff/sagas.js index aba57e02e..1ca8b1da5 100644 --- a/src/plugins/signoff/sagas.js +++ b/src/plugins/signoff/sagas.js @@ -145,8 +145,7 @@ function _updateCollectionAttributes(getState, data) { } = getState(); const coll = client.bucket(bid).collection(cid); return coll.setData(data, {safe: true, patch: true, last_modified}) - .then(() => { - return coll.getData(); - }) + .then(() => coll.getData()) + // FIXME: https://github.com/Kinto/kinto-http.js/issues/150 .then(attributes => ({data: attributes})); } From b4aadb37414d03714491452a72580d3c95001ea0 Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Thu, 17 Nov 2016 18:10:49 +0100 Subject: [PATCH 7/9] Add missing test file, slight refactor @leplatrem. --- src/plugins/signoff/components.js | 41 ++++----- test/plugins/signoff/sagas_test.js | 128 +++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 19 deletions(-) create mode 100644 test/plugins/signoff/sagas_test.js diff --git a/src/plugins/signoff/components.js b/src/plugins/signoff/components.js index 528b03962..e1cfe54f4 100644 --- a/src/plugins/signoff/components.js +++ b/src/plugins/signoff/components.js @@ -97,35 +97,38 @@ export default class SignoffToolBar extends React.Component { } const {source, preview, destination} = resource; + const canRequestReview = canEdit && isEditor(source, sessionState, bucketState); const canReview = canEdit && isReviewer(source, sessionState, bucketState) && !isLastEditor(source, sessionState); + const canSign = canEdit && isReviewer(source, sessionState, bucketState); // Default status is request review const step = status == "to-review" ? 1 : status == "signed" ? 2 : 0; return ( - + + step={1} + currentStep={step} + canEdit={canReview} + approveChanges={approveChanges} + declineChanges={declineChanges} + source={source} + preview={preview} /> + step={2} + currentStep={step} + canEdit={canSign} + reSign={approveChanges} + source={source} + destination={destination} /> ); } diff --git a/test/plugins/signoff/sagas_test.js b/test/plugins/signoff/sagas_test.js new file mode 100644 index 000000000..c6ef2f364 --- /dev/null +++ b/test/plugins/signoff/sagas_test.js @@ -0,0 +1,128 @@ +import { expect } from "chai"; +import { put } from "redux-saga/effects"; + +import { notifySuccess } from "../../../src/actions/notifications"; +import { routeLoadSuccess } from "../../../src/actions/route"; +import { setClient } from "../../../src/client"; +import * as actions from "../../../src/plugins/signoff/actions"; +import * as saga from "../../../src/plugins/signoff/sagas"; + + +describe("signoff plugin sagas", () => { + describe("handleRequestReview()", () => { + let bucket, collection, getState, handleRequestReview; + + before(() => { + collection = { + getData() {}, + setData() {}, + }; + bucket = {collection() {return collection;}}; + setClient({bucket() {return bucket;}}); + const action = actions.requestReview(); + getState = () => ({ + bucket: {data: {id: "buck"}}, + collection: {data: {id: "coll", last_modified: 42}} + }); + handleRequestReview = saga.handleRequestReview(getState, action); + }); + + it("should update the collection status as 'to-review'", () => { + expect(handleRequestReview.next({id: "coll"}).value) + .to.have.property("CALL") + .to.have.property("args") + .to.include({status: "to-review"}); + }); + + it("should dispatch the routeLoadSuccess action", () => { + expect(handleRequestReview.next({data: {id: "coll", status: "to-review"}}).value) + .eql(put(routeLoadSuccess({ + bucket: {data: {id: "buck"}}, + collection: {data: {id: "coll", status: "to-review"}}, + }))); + }); + + it("should dispatch a success notification", () => { + expect(handleRequestReview.next().value) + .eql(put(notifySuccess("Review requested."))); + }); + }); + + describe("handleDeclineChanges()", () => { + let bucket, collection, getState, handleDeclineChanges; + + before(() => { + collection = { + getData() {}, + setData() {}, + }; + bucket = {collection() {return collection;}}; + setClient({bucket() {return bucket;}}); + const action = actions.requestReview(); + getState = () => ({ + bucket: {data: {id: "buck"}}, + collection: {data: {id: "coll", last_modified: 42}} + }); + handleDeclineChanges = saga.handleDeclineChanges(getState, action); + }); + + it("should update the collection status as 'work-in-progress'", () => { + expect(handleDeclineChanges.next({id: "coll"}).value) + .to.have.property("CALL") + .to.have.property("args") + .to.include({status: "work-in-progress"}); + }); + + it("should dispatch the routeLoadSuccess action", () => { + expect(handleDeclineChanges.next({data: {id: "coll", status: "work-in-progress"}}).value) + .eql(put(routeLoadSuccess({ + bucket: {data: {id: "buck"}}, + collection: {data: {id: "coll", status: "work-in-progress"}}, + }))); + }); + + it("should dispatch a success notification", () => { + expect(handleDeclineChanges.next().value) + .eql(put(notifySuccess("Changes declined."))); + }); + }); + + describe("handleApproveChanges()", () => { + let bucket, collection, getState, handleApproveChanges; + + before(() => { + collection = { + getData() {}, + setData() {}, + }; + bucket = {collection() {return collection;}}; + setClient({bucket() {return bucket;}}); + const action = actions.requestReview(); + getState = () => ({ + bucket: {data: {id: "buck"}}, + collection: {data: {id: "coll", last_modified: 42}} + }); + handleApproveChanges = saga.handleApproveChanges(getState, action); + }); + + it("should update the collection status as 'to-sign'", () => { + expect(handleApproveChanges.next({id: "coll"}).value) + .to.have.property("CALL") + .to.have.property("args") + .to.include({status: "to-sign"}); + }); + + it("should dispatch the routeLoadSuccess action", () => { + expect(handleApproveChanges.next({data: {id: "coll", status: "to-sign"}}).value) + .eql(put(routeLoadSuccess({ + bucket: {data: {id: "buck"}}, + collection: {data: {id: "coll", status: "to-sign"}}, + }))); + }); + + it("should dispatch a success notification", () => { + expect(handleApproveChanges.next().value) + .eql(put(notifySuccess("Signature requested."))); + }); + }); +}); From 01240de462b8bd8321d6b291834549ab8da0d806 Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Thu, 17 Nov 2016 19:02:04 +0100 Subject: [PATCH 8/9] @leplatrem review. --- src/plugins/signoff/components.js | 13 +++++----- test/plugins/signoff/sagas_test.js | 39 ++++++++++++------------------ 2 files changed, 22 insertions(+), 30 deletions(-) diff --git a/src/plugins/signoff/components.js b/src/plugins/signoff/components.js index e1cfe54f4..7763d385a 100644 --- a/src/plugins/signoff/components.js +++ b/src/plugins/signoff/components.js @@ -19,19 +19,18 @@ import AdminLink from "../../components/AdminLink"; import { ProgressBar, ProgressStep } from "./ProgressBar.js"; -function isMember(groupkey, source, sessionState, bucketState) { +function isMember(groupKey, source, sessionState, bucketState) { const {serverInfo: {user={}, capabilities}} = sessionState; if (!user.id) { return false; } const {signer={}} = capabilities; - const {[groupkey]: defaultGroupName} = signer; - const {[groupkey]: groupName=defaultGroupName} = source; + const {[groupKey]: defaultGroupName} = signer; + const {[groupKey]: groupName=defaultGroupName} = source; const {id: userId} = user; - console.log(bucketState); const {groups} = bucketState; - const editorGroup = groups.find(g => g.id === groupName); - if (editorGroup == null) { + const group = groups.find(g => g.id === groupName); + if (group == null) { // XXX for now if we can't access the group it's probably because the user // doesn't have the permission to read it, so we mark the user has a member // of the group. @@ -40,7 +39,7 @@ function isMember(groupkey, source, sessionState, bucketState) { // be able to properly check for membership. return true; } - return editorGroup.members.includes(userId); + return group.members.includes(userId); } function isEditor(source, sessionState, bucketState) { diff --git a/test/plugins/signoff/sagas_test.js b/test/plugins/signoff/sagas_test.js index c6ef2f364..00a033439 100644 --- a/test/plugins/signoff/sagas_test.js +++ b/test/plugins/signoff/sagas_test.js @@ -9,16 +9,21 @@ import * as saga from "../../../src/plugins/signoff/sagas"; describe("signoff plugin sagas", () => { + let bucket, collection, getState; + + beforeEach(() => { + collection = { + getData() {}, + setData() {}, + }; + bucket = {collection() {return collection;}}; + setClient({bucket() {return bucket;}}); + }); + describe("handleRequestReview()", () => { - let bucket, collection, getState, handleRequestReview; + let handleRequestReview; before(() => { - collection = { - getData() {}, - setData() {}, - }; - bucket = {collection() {return collection;}}; - setClient({bucket() {return bucket;}}); const action = actions.requestReview(); getState = () => ({ bucket: {data: {id: "buck"}}, @@ -49,16 +54,10 @@ describe("signoff plugin sagas", () => { }); describe("handleDeclineChanges()", () => { - let bucket, collection, getState, handleDeclineChanges; + let handleDeclineChanges; before(() => { - collection = { - getData() {}, - setData() {}, - }; - bucket = {collection() {return collection;}}; - setClient({bucket() {return bucket;}}); - const action = actions.requestReview(); + const action = actions.declineChanges(); getState = () => ({ bucket: {data: {id: "buck"}}, collection: {data: {id: "coll", last_modified: 42}} @@ -88,16 +87,10 @@ describe("signoff plugin sagas", () => { }); describe("handleApproveChanges()", () => { - let bucket, collection, getState, handleApproveChanges; + let handleApproveChanges; before(() => { - collection = { - getData() {}, - setData() {}, - }; - bucket = {collection() {return collection;}}; - setClient({bucket() {return bucket;}}); - const action = actions.requestReview(); + const action = actions.approveChanges(); getState = () => ({ bucket: {data: {id: "buck"}}, collection: {data: {id: "coll", last_modified: 42}} From 34f9f23be98d8524c8c38fa259881e128cb17590 Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Thu, 17 Nov 2016 19:05:04 +0100 Subject: [PATCH 9/9] Moar test setup factoring. --- test/plugins/signoff/sagas_test.js | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/test/plugins/signoff/sagas_test.js b/test/plugins/signoff/sagas_test.js index 00a033439..0bf69d99c 100644 --- a/test/plugins/signoff/sagas_test.js +++ b/test/plugins/signoff/sagas_test.js @@ -11,13 +11,17 @@ import * as saga from "../../../src/plugins/signoff/sagas"; describe("signoff plugin sagas", () => { let bucket, collection, getState; - beforeEach(() => { + before(() => { collection = { getData() {}, setData() {}, }; bucket = {collection() {return collection;}}; setClient({bucket() {return bucket;}}); + getState = () => ({ + bucket: {data: {id: "buck"}}, + collection: {data: {id: "coll", last_modified: 42}} + }); }); describe("handleRequestReview()", () => { @@ -25,10 +29,6 @@ describe("signoff plugin sagas", () => { before(() => { const action = actions.requestReview(); - getState = () => ({ - bucket: {data: {id: "buck"}}, - collection: {data: {id: "coll", last_modified: 42}} - }); handleRequestReview = saga.handleRequestReview(getState, action); }); @@ -58,10 +58,6 @@ describe("signoff plugin sagas", () => { before(() => { const action = actions.declineChanges(); - getState = () => ({ - bucket: {data: {id: "buck"}}, - collection: {data: {id: "coll", last_modified: 42}} - }); handleDeclineChanges = saga.handleDeclineChanges(getState, action); }); @@ -91,10 +87,6 @@ describe("signoff plugin sagas", () => { before(() => { const action = actions.approveChanges(); - getState = () => ({ - bucket: {data: {id: "buck"}}, - collection: {data: {id: "coll", last_modified: 42}} - }); handleApproveChanges = saga.handleApproveChanges(getState, action); });