[Fix] Read only users should not be able to open a Relation picker (#10666)

## Context
ActivityTargetsInlineCell was not using the useIsFieldValueReadOnly hook
but a dedicated prop instead. To align with the rest of the
implementation I've updated that part of the code however we still want
the table/kanban views to always be in read-only mode regardless of the
hook logic so I've updated the hook to take the ContextStoreViewType
into account.
This commit is contained in:
Weiko
2025-03-05 13:56:58 +01:00
committed by GitHub
parent da0b0a9e3e
commit fd8b130df6
9 changed files with 102 additions and 42 deletions

View File

@ -13,6 +13,7 @@ import { CoreObjectNameSingular } from '@/object-metadata/types/CoreObjectNameSi
import { useFieldContext } from '@/object-record/hooks/useFieldContext'; import { useFieldContext } from '@/object-record/hooks/useFieldContext';
import { FieldContext } from '@/object-record/record-field/contexts/FieldContext'; import { FieldContext } from '@/object-record/record-field/contexts/FieldContext';
import { FieldFocusContextProvider } from '@/object-record/record-field/contexts/FieldFocusContextProvider'; import { FieldFocusContextProvider } from '@/object-record/record-field/contexts/FieldFocusContextProvider';
import { useIsFieldValueReadOnly } from '@/object-record/record-field/hooks/useIsFieldValueReadOnly';
import { RecordFieldInputScope } from '@/object-record/record-field/scopes/RecordFieldInputScope'; import { RecordFieldInputScope } from '@/object-record/record-field/scopes/RecordFieldInputScope';
import { RecordInlineCellContainer } from '@/object-record/record-inline-cell/components/RecordInlineCellContainer'; import { RecordInlineCellContainer } from '@/object-record/record-inline-cell/components/RecordInlineCellContainer';
import { RecordInlineCellContext } from '@/object-record/record-inline-cell/components/RecordInlineCellContext'; import { RecordInlineCellContext } from '@/object-record/record-inline-cell/components/RecordInlineCellContext';
@ -23,7 +24,6 @@ type ActivityTargetsInlineCellProps = {
activity: Task | Note; activity: Task | Note;
showLabel?: boolean; showLabel?: boolean;
maxWidth?: number; maxWidth?: number;
readonly?: boolean;
activityObjectNameSingular: activityObjectNameSingular:
| CoreObjectNameSingular.Note | CoreObjectNameSingular.Note
| CoreObjectNameSingular.Task; | CoreObjectNameSingular.Task;
@ -33,7 +33,6 @@ export const ActivityTargetsInlineCell = ({
activity, activity,
showLabel = true, showLabel = true,
maxWidth, maxWidth,
readonly,
activityObjectNameSingular, activityObjectNameSingular,
}: ActivityTargetsInlineCellProps) => { }: ActivityTargetsInlineCellProps) => {
const { activityTargetObjectRecords } = const { activityTargetObjectRecords } =
@ -43,6 +42,8 @@ export const ActivityTargetsInlineCell = ({
const { fieldDefinition } = useContext(FieldContext); const { fieldDefinition } = useContext(FieldContext);
const isFieldReadOnly = useIsFieldValueReadOnly();
useScopedHotkeys( useScopedHotkeys(
Key.Escape, Key.Escape,
() => { () => {
@ -76,7 +77,7 @@ export const ActivityTargetsInlineCell = ({
}, },
IconLabel: showLabel ? IconArrowUpRight : undefined, IconLabel: showLabel ? IconArrowUpRight : undefined,
showLabel: showLabel, showLabel: showLabel,
readonly: readonly, readonly: isFieldReadOnly,
labelWidth: fieldDefinition?.labelWidth, labelWidth: fieldDefinition?.labelWidth,
editModeContent: ( editModeContent: (
<ActivityTargetInlineCellEditMode <ActivityTargetInlineCellEditMode

View File

@ -95,7 +95,6 @@ export const NoteCard = ({
<ActivityTargetsInlineCell <ActivityTargetsInlineCell
activity={note} activity={note}
activityObjectNameSingular={CoreObjectNameSingular.Note} activityObjectNameSingular={CoreObjectNameSingular.Note}
readonly
/> />
</NoteTargetsContextProvider> </NoteTargetsContextProvider>
)} )}

View File

@ -134,7 +134,6 @@ export const TaskRow = ({ task }: { task: Task }) => {
activity={task} activity={task}
showLabel={false} showLabel={false}
maxWidth={200} maxWidth={200}
readonly
/> />
</TaskTargetsContextProvider> </TaskTargetsContextProvider>
)} )}

View File

@ -2,6 +2,7 @@ import { renderHook } from '@testing-library/react';
import { ReactNode } from 'react'; import { ReactNode } from 'react';
import { RecoilRoot } from 'recoil'; import { RecoilRoot } from 'recoil';
import { ContextStoreComponentInstanceContext } from '@/context-store/states/contexts/ContextStoreComponentInstanceContext';
import { import {
actorFieldDefinition, actorFieldDefinition,
phonesFieldDefinition, phonesFieldDefinition,
@ -17,34 +18,39 @@ import { JestRecordStoreSetter } from '~/testing/jest/JestRecordStoreSetter';
import { useIsFieldValueReadOnly } from '../useIsFieldValueReadOnly'; import { useIsFieldValueReadOnly } from '../useIsFieldValueReadOnly';
const recordId = 'recordId'; const recordId = 'recordId';
const mockInstanceId = 'mock-instance-id';
const getWrapper = const getWrapper =
(fieldDefinition: FieldDefinition<FieldMetadata>, isRecordDeleted: boolean) => (fieldDefinition: FieldDefinition<FieldMetadata>, isRecordDeleted: boolean) =>
({ children }: { children: ReactNode }) => { ({ children }: { children: ReactNode }) => {
return ( return (
<RecoilRoot> <RecoilRoot>
<JestObjectMetadataItemSetter> <ContextStoreComponentInstanceContext.Provider
<JestRecordStoreSetter value={{ instanceId: mockInstanceId }}
records={[ >
{ <JestObjectMetadataItemSetter>
id: recordId, <JestRecordStoreSetter
deletedAt: isRecordDeleted ? new Date().toISOString() : null, records={[
__typename: 'standardObject', {
} as ObjectRecord, id: recordId,
]} deletedAt: isRecordDeleted ? new Date().toISOString() : null,
> __typename: 'standardObject',
<FieldContext.Provider } as ObjectRecord,
value={{ ]}
fieldDefinition,
recordId,
hotkeyScope: 'hotkeyScope',
isLabelIdentifier: false,
}}
> >
{children} <FieldContext.Provider
</FieldContext.Provider> value={{
</JestRecordStoreSetter> fieldDefinition,
</JestObjectMetadataItemSetter> recordId,
hotkeyScope: 'hotkeyScope',
isLabelIdentifier: false,
}}
>
{children}
</FieldContext.Provider>
</JestRecordStoreSetter>
</JestObjectMetadataItemSetter>
</ContextStoreComponentInstanceContext.Provider>
</RecoilRoot> </RecoilRoot>
); );
}; };

View File

@ -1,9 +1,11 @@
import { useContext } from 'react'; import { useContext } from 'react';
import { contextStoreCurrentViewTypeComponentState } from '@/context-store/states/contextStoreCurrentViewTypeComponentState';
import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem'; import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem';
import { recordStoreFamilyState } from '@/object-record/record-store/states/recordStoreFamilyState'; import { recordStoreFamilyState } from '@/object-record/record-store/states/recordStoreFamilyState';
import { ObjectRecord } from '@/object-record/types/ObjectRecord'; import { ObjectRecord } from '@/object-record/types/ObjectRecord';
import { useHasObjectReadOnlyPermission } from '@/settings/roles/hooks/useHasObjectReadOnlyPermission'; import { useHasObjectReadOnlyPermission } from '@/settings/roles/hooks/useHasObjectReadOnlyPermission';
import { useRecoilComponentValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentValueV2';
import { useRecoilValue } from 'recoil'; import { useRecoilValue } from 'recoil';
import { FieldContext } from '../contexts/FieldContext'; import { FieldContext } from '../contexts/FieldContext';
import { isFieldValueReadOnly } from '../utils/isFieldValueReadOnly'; import { isFieldValueReadOnly } from '../utils/isFieldValueReadOnly';
@ -17,6 +19,10 @@ export const useIsFieldValueReadOnly = () => {
recordStoreFamilyState(recordId), recordStoreFamilyState(recordId),
); );
const contextStoreCurrentViewType = useRecoilComponentValueV2(
contextStoreCurrentViewTypeComponentState,
);
const { objectMetadataItem } = useObjectMetadataItem({ const { objectMetadataItem } = useObjectMetadataItem({
objectNameSingular: metadata.objectMetadataNameSingular ?? '', objectNameSingular: metadata.objectMetadataNameSingular ?? '',
}); });
@ -30,5 +36,6 @@ export const useIsFieldValueReadOnly = () => {
isObjectRemote: objectMetadataItem.isRemote, isObjectRemote: objectMetadataItem.isRemote,
isRecordDeleted: recordFromStore?.deletedAt, isRecordDeleted: recordFromStore?.deletedAt,
hasObjectReadOnlyPermission, hasObjectReadOnlyPermission,
contextStoreCurrentViewType,
}); });
}; };

View File

@ -1,3 +1,4 @@
import { ContextStoreViewType } from '@/context-store/types/ContextStoreViewType';
import { isFieldValueReadOnly } from '@/object-record/record-field/utils/isFieldValueReadOnly'; import { isFieldValueReadOnly } from '@/object-record/record-field/utils/isFieldValueReadOnly';
import { FieldMetadataType } from '~/generated/graphql'; import { FieldMetadataType } from '~/generated/graphql';
@ -5,19 +6,37 @@ describe('isFieldValueReadOnly', () => {
it('should return true if fieldName is noteTargets or taskTargets', () => { it('should return true if fieldName is noteTargets or taskTargets', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
fieldName: 'noteTargets', fieldName: 'noteTargets',
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(true); expect(result).toBe(true);
const result2 = isFieldValueReadOnly({ const result2 = isFieldValueReadOnly({
fieldName: 'taskTargets', fieldName: 'taskTargets',
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result2).toBe(true); expect(result2).toBe(true);
}); });
it('should return true if fieldName is noteTargets or taskTargets but is not in table or kanban view', () => {
const result = isFieldValueReadOnly({
fieldName: 'noteTargets',
contextStoreCurrentViewType: ContextStoreViewType.ShowPage,
});
expect(result).toBe(false);
const result2 = isFieldValueReadOnly({
fieldName: 'taskTargets',
contextStoreCurrentViewType: ContextStoreViewType.ShowPage,
});
expect(result2).toBe(false);
});
it('should return false if fieldName is not noteTargets or taskTargets', () => { it('should return false if fieldName is not noteTargets or taskTargets', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
fieldName: 'test', fieldName: 'test',
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(false); expect(result).toBe(false);
@ -26,6 +45,7 @@ describe('isFieldValueReadOnly', () => {
it('should return true if isObjectRemote is true', () => { it('should return true if isObjectRemote is true', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
isObjectRemote: true, isObjectRemote: true,
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(true); expect(result).toBe(true);
@ -34,6 +54,7 @@ describe('isFieldValueReadOnly', () => {
it('should return false if isObjectRemote is false', () => { it('should return false if isObjectRemote is false', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
isObjectRemote: false, isObjectRemote: false,
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(false); expect(result).toBe(false);
@ -42,6 +63,7 @@ describe('isFieldValueReadOnly', () => {
it('should return true if isRecordDeleted is true', () => { it('should return true if isRecordDeleted is true', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
isRecordDeleted: true, isRecordDeleted: true,
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(true); expect(result).toBe(true);
@ -50,6 +72,7 @@ describe('isFieldValueReadOnly', () => {
it('should return false if isRecordDeleted is false', () => { it('should return false if isRecordDeleted is false', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
isRecordDeleted: false, isRecordDeleted: false,
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(false); expect(result).toBe(false);
@ -59,6 +82,7 @@ describe('isFieldValueReadOnly', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
objectNameSingular: 'workflow', objectNameSingular: 'workflow',
fieldName: 'test', fieldName: 'test',
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(true); expect(result).toBe(true);
@ -68,6 +92,7 @@ describe('isFieldValueReadOnly', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
objectNameSingular: 'Workflow', objectNameSingular: 'Workflow',
fieldName: 'name', fieldName: 'name',
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(false); expect(result).toBe(false);
@ -76,6 +101,7 @@ describe('isFieldValueReadOnly', () => {
it('should return true if isWorkflowSubObjectMetadata is true', () => { it('should return true if isWorkflowSubObjectMetadata is true', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
objectNameSingular: 'workflowVersion', objectNameSingular: 'workflowVersion',
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(true); expect(result).toBe(true);
@ -84,6 +110,7 @@ describe('isFieldValueReadOnly', () => {
it('should return true if fieldType is FieldMetadataType.ACTOR', () => { it('should return true if fieldType is FieldMetadataType.ACTOR', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
fieldType: FieldMetadataType.ACTOR, fieldType: FieldMetadataType.ACTOR,
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(true); expect(result).toBe(true);
@ -92,6 +119,7 @@ describe('isFieldValueReadOnly', () => {
it('should return true if fieldType is FieldMetadataType.RICH_TEXT', () => { it('should return true if fieldType is FieldMetadataType.RICH_TEXT', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
fieldType: FieldMetadataType.RICH_TEXT, fieldType: FieldMetadataType.RICH_TEXT,
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(true); expect(result).toBe(true);
@ -100,13 +128,16 @@ describe('isFieldValueReadOnly', () => {
it('should return false if fieldType is not FieldMetadataType.ACTOR or FieldMetadataType.RICH_TEXT', () => { it('should return false if fieldType is not FieldMetadataType.ACTOR or FieldMetadataType.RICH_TEXT', () => {
const result = isFieldValueReadOnly({ const result = isFieldValueReadOnly({
fieldType: FieldMetadataType.TEXT, fieldType: FieldMetadataType.TEXT,
contextStoreCurrentViewType: ContextStoreViewType.Table,
}); });
expect(result).toBe(false); expect(result).toBe(false);
}); });
it('should return false if none of the conditions are met', () => { it('should return false if none of the conditions are met', () => {
const result = isFieldValueReadOnly({}); const result = isFieldValueReadOnly({
contextStoreCurrentViewType: ContextStoreViewType.Table,
});
expect(result).toBe(false); expect(result).toBe(false);
}); });

View File

@ -1,3 +1,4 @@
import { ContextStoreViewType } from '@/context-store/types/ContextStoreViewType';
import { CoreObjectNameSingular } from '@/object-metadata/types/CoreObjectNameSingular'; import { CoreObjectNameSingular } from '@/object-metadata/types/CoreObjectNameSingular';
import { isWorkflowSubObjectMetadata } from '@/object-metadata/utils/isWorkflowSubObjectMetadata'; import { isWorkflowSubObjectMetadata } from '@/object-metadata/utils/isWorkflowSubObjectMetadata';
import { isFieldActor } from '@/object-record/record-field/types/guards/isFieldActor'; import { isFieldActor } from '@/object-record/record-field/types/guards/isFieldActor';
@ -13,6 +14,7 @@ type isFieldValueReadOnlyParams = {
isObjectRemote?: boolean; isObjectRemote?: boolean;
isRecordDeleted?: boolean; isRecordDeleted?: boolean;
hasObjectReadOnlyPermission?: boolean; hasObjectReadOnlyPermission?: boolean;
contextStoreCurrentViewType: ContextStoreViewType | null;
}; };
export const isFieldValueReadOnly = ({ export const isFieldValueReadOnly = ({
@ -22,8 +24,16 @@ export const isFieldValueReadOnly = ({
isObjectRemote = false, isObjectRemote = false,
isRecordDeleted = false, isRecordDeleted = false,
hasObjectReadOnlyPermission = false, hasObjectReadOnlyPermission = false,
contextStoreCurrentViewType,
}: isFieldValueReadOnlyParams) => { }: isFieldValueReadOnlyParams) => {
if (fieldName === 'noteTargets' || fieldName === 'taskTargets') { const isTableViewOrKanbanView =
contextStoreCurrentViewType === ContextStoreViewType.Table ||
contextStoreCurrentViewType === ContextStoreViewType.Kanban;
const isTargetField =
fieldName === 'noteTargets' || fieldName === 'taskTargets';
if (isTableViewOrKanbanView && isTargetField) {
return true; return true;
} }

View File

@ -1,3 +1,4 @@
import { ContextStoreViewType } from '@/context-store/types/ContextStoreViewType';
import { useGetStandardObjectIcon } from '@/object-metadata/hooks/useGetStandardObjectIcon'; import { useGetStandardObjectIcon } from '@/object-metadata/hooks/useGetStandardObjectIcon';
import { CoreObjectNameSingular } from '@/object-metadata/types/CoreObjectNameSingular'; import { CoreObjectNameSingular } from '@/object-metadata/types/CoreObjectNameSingular';
import { FieldContext } from '@/object-record/record-field/contexts/FieldContext'; import { FieldContext } from '@/object-record/record-field/contexts/FieldContext';
@ -53,6 +54,7 @@ export const SummaryCard = ({
const isReadOnly = isFieldValueReadOnly({ const isReadOnly = isFieldValueReadOnly({
objectNameSingular, objectNameSingular,
isRecordDeleted: recordFromStore?.isDeleted, isRecordDeleted: recordFromStore?.isDeleted,
contextStoreCurrentViewType: ContextStoreViewType.ShowPage,
}); });
const isCommandMenuV2Enabled = useIsFeatureEnabled( const isCommandMenuV2Enabled = useIsFeatureEnabled(

View File

@ -10,6 +10,7 @@ import { SnackBarDecorator } from '~/testing/decorators/SnackBarDecorator';
import { graphqlMocks } from '~/testing/graphqlMocks'; import { graphqlMocks } from '~/testing/graphqlMocks';
import { getCompaniesMock } from '~/testing/mock-data/companies'; import { getCompaniesMock } from '~/testing/mock-data/companies';
import { ContextStoreComponentInstanceContext } from '@/context-store/states/contexts/ContextStoreComponentInstanceContext';
import { I18nFrontDecorator } from '~/testing/decorators/I18nFrontDecorator'; import { I18nFrontDecorator } from '~/testing/decorators/I18nFrontDecorator';
import { generatedMockObjectMetadataItems } from '~/testing/mock-data/generatedMockObjectMetadataItems'; import { generatedMockObjectMetadataItems } from '~/testing/mock-data/generatedMockObjectMetadataItems';
import { allMockPersonRecords } from '~/testing/mock-data/people'; import { allMockPersonRecords } from '~/testing/mock-data/people';
@ -31,21 +32,25 @@ const meta: Meta<typeof RecordDetailRelationSection> = {
component: RecordDetailRelationSection, component: RecordDetailRelationSection,
decorators: [ decorators: [
(Story) => ( (Story) => (
<FieldContext.Provider <ContextStoreComponentInstanceContext.Provider
value={{ value={{ instanceId: 'mock-instance-id' }}
recordId: companiesMock[0].id,
isLabelIdentifier: false,
fieldDefinition: formatFieldMetadataItemAsFieldDefinition({
field: mockedCompanyObjectMetadataItem.fields.find(
({ name }) => name === 'people',
)!,
objectMetadataItem: mockedCompanyObjectMetadataItem,
}),
hotkeyScope: 'hotkey-scope',
}}
> >
<Story /> <FieldContext.Provider
</FieldContext.Provider> value={{
recordId: companiesMock[0].id,
isLabelIdentifier: false,
fieldDefinition: formatFieldMetadataItemAsFieldDefinition({
field: mockedCompanyObjectMetadataItem.fields.find(
({ name }) => name === 'people',
)!,
objectMetadataItem: mockedCompanyObjectMetadataItem,
}),
hotkeyScope: 'hotkey-scope',
}}
>
<Story />
</FieldContext.Provider>
</ContextStoreComponentInstanceContext.Provider>
), ),
ComponentDecorator, ComponentDecorator,
ObjectMetadataItemsDecorator, ObjectMetadataItemsDecorator,