Add readOnly in curation page #481
Conversation
There was a problem hiding this comment.
Thanks for making the changes!
Would you also try adding a banner to the curation page that alerts the user that the page is currently in read only mode?
Some text along the lines of "This page is currently in read-only mode because ___ user is currently reviewing"
| @@ -13,9 +13,16 @@ import { Button } from 'reactstrap'; | |||
| export interface IRCTButtonProps extends StoreProps { | |||
| cancerTypePath: string; | |||
| relevantCancerTypesInfoPath: string; // path to dx, px, or tx | |||
| readOnly?: boolean; | |||
There was a problem hiding this comment.
We don't need to define the readOnly property here because it will come from StoreProp type.
There was a problem hiding this comment.
I added the readOnly property to RCTButton because I thought this component might be used in multiple places simultaneously. If it’s only used in CurationPage, I’ll remove this line and use StoreProp instead.
| @@ -204,19 +211,21 @@ function CancerTypeCollapsible({ | |||
| action={ | |||
| <> | |||
| <CommentIcon id={`${cancerTypesUuid}_diagnostic_comments`} path={`${cancerTypePath}/diagnostic_comments`} /> | |||
| <RCTButton cancerTypePath={cancerTypePath} relevantCancerTypesInfoPath={`${cancerTypePath}/diagnostic`} /> | |||
| <RCTButton cancerTypePath={cancerTypePath} relevantCancerTypesInfoPath={`${cancerTypePath}/diagnostic`} readOnly={readOnly} /> | |||
There was a problem hiding this comment.
Passing in the readOnly props explicitly is not required because it will be injected by mobx.
I have added a banner to the top of the page. Please let me know if there are any improvements needed for the text or style. |
Let's get rid of the "x" button - the alert should not be dismissible. |
Update of banner |
| <span> | ||
| {isGermline ? 'This Germline ' : 'This Somatic '} | ||
| is currently in read only mode because {meta.review.currentReviewer} is currently reviewing. | ||
| </span> |
There was a problem hiding this comment.
Maybe The germline section is current in read only mode because {meta.review.currentReviewer} is currently reviewing.
| @@ -86,6 +88,11 @@ export const CurationPage = (props: ICurationPageProps) => { | |||
| } | |||
| if (geneEntity && props.firebaseInitSuccess) { | |||
| const cleanupCallbacks: Unsubscribe[] = []; | |||
| cleanupCallbacks.push( | |||
| onValue(ref(props.firebaseDb, firebaseMetaCurrentReviewerPath), snapshot => { | |||
| props.setReadOnly(snapshot.val() ? true : false); | |||
There was a problem hiding this comment.
Can keep it concise using double negation
| props.setReadOnly(snapshot.val() ? true : false); | |
| props.setReadOnly(!!snapshot.val()); |
| @@ -7,9 +7,11 @@ const AddMutationButton: React.FunctionComponent<{ | |||
| onClickHandler: (show: boolean) => void; | |||
| showIcon?: boolean; | |||
| showFullTitle?: boolean; | |||
| }> = ({ showAddMutationModal, onClickHandler, showIcon = true, showFullTitle = false }) => { | |||
| disabled?: boolean; | |||
There was a problem hiding this comment.
I would keep it consistent with the RCTButton and just inject the CurationPage store into AddMutationButton and use the readOnly property for disabled. Same end result, but one less prop to pass in.
| const ReadOnlyBanner = ({ isGermline, hugoSymbol, firebaseDb }: IReadOnlyBanner) => { | ||
| const firebaseMetaPath = getFirebaseMetaGenePath(isGermline, hugoSymbol); | ||
| const [meta, setMeta] = useState<Meta>(); | ||
| const [isVisible, setIsVisible] = useState(true); |
There was a problem hiding this comment.
Seems like isVisible is not needed anymore because we always display the banner.
| @@ -133,6 +137,10 @@ export class CurationPageStore { | |||
| .filter(query => query.queryId && this.annotatedAltsCache.cache[query.queryId] && this.annotatedAltsCache.cache[query.queryId].result) | |||
| .map(query => (query.queryId !== undefined ? this.annotatedAltsCache.cache[query.queryId].result : undefined)); | |||
| } | |||
|
|
|||
| setReadOnly(isAnotherUserReviewing: boolean) { | |||
| this.readOnly = isAnotherUserReviewing ? true : false; | |||
There was a problem hiding this comment.
Already a boolean value, can just set as is.
| this.readOnly = isAnotherUserReviewing ? true : false; | |
| this.readOnly = isAnotherUserReviewing; |
| } | ||
|
|
||
| const ReadOnlyBanner = ({ isGermline, hugoSymbol, firebaseDb }: IReadOnlyBanner) => { | ||
| const firebaseMetaPath = getFirebaseMetaGenePath(isGermline, hugoSymbol); |
There was a problem hiding this comment.
Please put this inside the useEffect and add isGermline and hugoSymbol to the dependency array.
If the path changes you'd would want the useEffect to be called again.
Fixes https://github.com/oncokb/oncokb-pipeline/issues/236