-
-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Set Planter Manager not update the selfie without Tree Manager Role #83
Conversation
@nmcharlton I have also added another PR in Admin API |
src/components/EditPlanter.js
Outdated
@@ -1,4 +1,4 @@ | |||
import React, { useState, useEffect } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to make any changes to this file.
These changes can be reverted.
src/components/EditPlanter.js
Outdated
@@ -91,7 +94,16 @@ const EditPlanter = (props) => { | |||
} | |||
|
|||
function handleSelectPlanterImage(img) { | |||
handleChange('imageUrl', img); | |||
// Planter Manager with Tree Manager role cannot set planter selfie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct.
A planter manager should be able to set this image even if they don't have tree permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wrote wrong comment. It should be "Planter Manager without Tree Manager role cannot set planter selfie".
So what you mean I should hangle logic when clicking "Save" button instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean that we don’t need to check permissions at all.
The title of the issue is a description of the bug, not the desired functionality.
src/api/planters.js
Outdated
@@ -83,12 +83,13 @@ export default { | |||
order: 'timeUpdated DESC', | |||
limit: 100, | |||
fields: ['planterPhotoUrl'], | |||
where: { planterId, planterPhotoUrl: { neq: null } }, | |||
}; | |||
|
|||
const treeQuery = `${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense to call this treeQuery
any more, as it's querying planter selfies.
The client doesn't need to know that the api is using the trees database for this lookup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Resolves issue #57