Skip to content
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: UX Document Conversion in multi worker environment #1378

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
0dd028d
first functionnal POC
fflorent Jun 12, 2024
eafa1fd
feat: update UX according figma
hexaltation Aug 20, 2024
6b6e715
wip: working implementation
hexaltation Aug 22, 2024
170d79c
fix: Radio Button
hexaltation Aug 26, 2024
671f12b
chore: create transaltion keys
hexaltation Aug 26, 2024
3fd0d19
tests ADDED
hexaltation Aug 27, 2024
d7e4bc2
tests ADDED
hexaltation Aug 27, 2024
4cf6084
fix: to small time value for waiting page reload
hexaltation Aug 27, 2024
12f65dc
fix: after lsebille review
hexaltation Aug 29, 2024
9180cbd
fix: following fflorent review
hexaltation Sep 12, 2024
cb473d7
fix: invalid Document types
hexaltation Oct 3, 2024
f5d713d
fix: removing wheel reinvention of urlStates
hexaltation Oct 7, 2024
dd3d9b0
tests: detection of save copy button
hexaltation Oct 7, 2024
1585e55
wip: fiddle
hexaltation Oct 9, 2024
9e8dc9c
fix: inconsistent tooltip test
hexaltation Oct 9, 2024
f6e65ab
chore: code factorization
hexaltation Oct 9, 2024
db25153
chore: factorize tests
hexaltation Oct 9, 2024
0f63c2b
fix: minor typos
hexaltation Oct 9, 2024
d41b12d
fix: waiting time to pass CI
hexaltation Oct 10, 2024
b0524f8
style: Move all styling linked to document settings enhancement in an…
hexaltation Oct 31, 2024
19ef0d6
fix: _buildDocumentTypeModal
hexaltation Oct 31, 2024
0d5d7d5
fix: ADD DOCTYPE_XXX symbols
hexaltation Oct 31, 2024
6ec61a2
fix: move persist function to UserAPI class
hexaltation Oct 31, 2024
5941575
fix: empty string substitution for null doctype in PATCH doc
hexaltation Oct 31, 2024
e74d7f6
fix: test after chrome driver updated to 130
hexaltation Nov 4, 2024
5e47a66
feat: Change wording according to @dsagal suggestion in Issue #1015
hexaltation Nov 4, 2024
4150302
fix: check type key possible values in PATCH /api/docs/{did}
hexaltation Nov 4, 2024
a7d1173
fix: wording according to jr-grist remark on Issue #1015
hexaltation Nov 4, 2024
74aa91d
fix: title and radio button margin
hexaltation Nov 18, 2024
2e30d74
fix: ADD missing translation key
hexaltation Nov 19, 2024
8dea539
feat: add tooltip on the feature
hexaltation Nov 25, 2024
ef5145c
fix: wrongly formatted json
hexaltation Nov 25, 2024
78cca12
revert: remove tooltip
hexaltation Nov 26, 2024
913c45d
FIX: Document conversion work in multi-worker env
hexaltation Jan 14, 2025
e4756cf
fix: DOCTYPE_XXX better reuse
hexaltation Jan 14, 2025
4383b20
fix: according to @georgegevoian review
hexaltation Jan 16, 2025
3fa8ffd
fix: remove uneeded function persitType
hexaltation Jan 16, 2025
0965afa
fix: Move t() from top level to inside a function call
hexaltation Jan 18, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions app/client/models/DocPageModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,20 @@ import {buildUrlId, IGristUrlState, parseUrlId, UrlIdParts} from 'app/common/gri
import {getReconnectTimeout} from 'app/common/gutil';
import {canEdit, isOwner} from 'app/common/roles';
import {UserInfo} from 'app/common/User';
import {Document, NEW_DOCUMENT_CODE, Organization, UserAPI, Workspace} from 'app/common/UserAPI';
import {
DOCTYPE_TEMPLATE,
DOCTYPE_TUTORIAL,
Document,
NEW_DOCUMENT_CODE,
Organization,
UserAPI,
Workspace
} from 'app/common/UserAPI';
import {Holder, Observable, subscribe} from 'grainjs';
import {Computed, Disposable, dom, DomArg, DomElementArg} from 'grainjs';
import {makeT} from 'app/client/lib/localization';
import {logTelemetryEvent} from 'app/client/lib/telemetry';
import {DocumentType} from 'app/common/UserAPI';

// tslint:disable:no-console

Expand Down Expand Up @@ -87,7 +96,7 @@ export interface DocPageModel {
isTutorialTrunk: Observable<boolean>;
isTutorialFork: Observable<boolean>;
isTemplate: Observable<boolean>;

type: Observable<DocumentType>;
importSources: ImportSource[];

undoState: Observable<IUndoState|null>; // See UndoStack for details.
Expand Down Expand Up @@ -147,6 +156,8 @@ export class DocPageModelImpl extends Disposable implements DocPageModel {
(use, doc) => doc ? doc.isTutorialFork : false);
public readonly isTemplate = Computed.create(this, this.currentDoc,
(use, doc) => doc ? doc.isTemplate : false);
public readonly type = Computed.create(this, this.currentDoc,
(use, doc) => doc?.type ?? null);

public readonly importSources: ImportSource[] = [];

Expand Down Expand Up @@ -499,7 +510,8 @@ function buildDocInfo(doc: Document, mode: OpenDocMode | undefined): DocInfo {
const isFork = Boolean(idParts.forkId || idParts.snapshotId);
const isBareFork = isFork && idParts.trunkId === NEW_DOCUMENT_CODE;
const isSnapshot = Boolean(idParts.snapshotId);
const isTutorial = doc.type === 'tutorial';
const type = doc.type;
const isTutorial = type === DOCTYPE_TUTORIAL;
const isTutorialTrunk = isTutorial && !isFork && mode !== 'default';
const isTutorialFork = isTutorial && isFork;

Expand All @@ -511,7 +523,7 @@ function buildDocInfo(doc: Document, mode: OpenDocMode | undefined): DocInfo {
// mode. Since the document's 'openMode' has no effect, don't bother trying
// to set it here, as it'll potentially be confusing for other code reading it.
openMode = 'default';
} else if (!isFork && doc.type === 'template') {
} else if (!isFork && type === DOCTYPE_TEMPLATE) {
// Templates should always open in fork mode by default.
openMode = 'fork';
} else {
Expand All @@ -521,7 +533,7 @@ function buildDocInfo(doc: Document, mode: OpenDocMode | undefined): DocInfo {
}

const isPreFork = openMode === 'fork';
const isTemplate = doc.type === 'template' && (isFork || isPreFork);
const isTemplate = type === DOCTYPE_TEMPLATE && (isFork || isPreFork);
const isEditable = !isSnapshot && (canEdit(doc.access) || isPreFork);
return {
...doc,
Expand All @@ -534,6 +546,7 @@ function buildDocInfo(doc: Document, mode: OpenDocMode | undefined): DocInfo {
isSnapshot,
isTutorialTrunk,
isTutorialFork,
type,
isTemplate,
isReadonly: !isEditable,
idParts,
Expand Down
201 changes: 190 additions & 11 deletions app/client/ui/DocumentSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,18 @@ import {commonUrls, GristLoadConfig} from 'app/common/gristUrls';
import {not, propertyCompare} from 'app/common/gutil';
import {getCurrency, locales} from 'app/common/Locales';
import {isOwner, isOwnerOrEditor} from 'app/common/roles';
import {Computed, Disposable, dom, fromKo, IDisposableOwner, makeTestId, Observable, styled} from 'grainjs';
import {DOCTYPE_NORMAL, DOCTYPE_TEMPLATE, DOCTYPE_TUTORIAL, DocumentType} from 'app/common/UserAPI';
import {
Computed,
Disposable,
dom,
DomElementMethod,
fromKo,
IDisposableOwner,
makeTestId,
Observable,
styled
} from 'grainjs';
import * as moment from 'moment-timezone';

const t = makeT('DocumentSettings');
Expand Down Expand Up @@ -85,6 +96,22 @@ export class DocSettingsPage extends Disposable {
{defaultCurrencyLabel: t("Local currency ({{currency}})", {currency: getCurrency(l)})})
)
}),
dom.create(AdminSectionItem, {
id: 'templateMode',
name: t('Template mode'),
description: t('Change document type'),
value: cssDocTypeContainer(
dom.create(
displayCurrentType,
docPageModel.type,
),
cssSmallButton(t('Edit'),
dom.on('click', this._buildDocumentTypeModal.bind(this, true)),
hexaltation marked this conversation as resolved.
Show resolved Hide resolved
testId('doctype-edit')
),
),
disabled: isDocOwner ? false : t('Only available to document owners'),
}),
]),

dom.create(AdminSection, t('Data Engine'), [
Expand Down Expand Up @@ -120,15 +147,13 @@ export class DocSettingsPage extends Disposable {
)),
disabled: isDocOwner ? false : t('Only available to document owners'),
}),

dom.create(AdminSectionItem, {
id: 'reload',
name: t('Reload'),
description: t('Hard reset of data engine'),
value: cssSmallButton(t('Reload data engine'), dom.on('click', this._reloadEngine.bind(this, true))),
disabled: isDocEditor ? false : t('Only available to document editors'),
}),

canChangeEngine ? dom.create(AdminSectionItem, {
id: 'python',
name: t('Python'),
Expand Down Expand Up @@ -186,7 +211,6 @@ export class DocSettingsPage extends Disposable {
href: getApiConsoleLink(docPageModel),
}),
}),

dom.create(AdminSectionItem, {
id: 'webhooks',
name: t('Webhooks'),
Expand Down Expand Up @@ -224,11 +248,11 @@ export class DocSettingsPage extends Disposable {
const docPageModel = this._gristDoc.docPageModel;
modal((ctl, owner) => {
this.onDispose(() => ctl.close());
const selected = Observable.create<Option>(owner, Option.Adhoc);
const selected = Observable.create<TimingModalOption>(owner, TimingModalOption.Adhoc);
const page = Observable.create<TimingModalPage>(owner, TimingModalPage.Start);

const startTiming = async () => {
if (selected.get() === Option.Reload) {
if (selected.get() === TimingModalOption.Reload) {
page.set(TimingModalPage.Spinner);
await this._gristDoc.docApi.startTiming();
await docPageModel.appModel.api.getDocAPI(docPageModel.currentDocId.get()!).forceReload();
Expand All @@ -243,7 +267,7 @@ export class DocSettingsPage extends Disposable {
const startPage = () => [
cssRadioCheckboxOptions(
dom.style('max-width', '400px'),
radioCheckboxOption(selected, Option.Adhoc, dom('div',
radioCheckboxOption(selected, TimingModalOption.Adhoc, dom('div',
dom('div',
dom('strong', t('Start timing')),
),
Expand All @@ -253,7 +277,7 @@ export class DocSettingsPage extends Disposable {
),
testId('timing-modal-option-adhoc'),
)),
radioCheckboxOption(selected, Option.Reload, dom('div',
radioCheckboxOption(selected, TimingModalOption.Reload, dom('div',
dom('div',
dom('strong', t('Time reload')),
),
Expand Down Expand Up @@ -289,6 +313,111 @@ export class DocSettingsPage extends Disposable {
});
}

private _buildDocumentTypeModal() {
const docPageModel = this._gristDoc.docPageModel;
modal((ctl, owner) => {
this.onDispose(() => ctl.close());
const currentDocType = docPageModel.type.get() as string;
let currentDocTypeOption;
switch (currentDocType) {
case DOCTYPE_TEMPLATE:
currentDocTypeOption = DocTypeOption.Template;
break;
case DOCTYPE_TUTORIAL:
currentDocTypeOption = DocTypeOption.Tutorial;
break;
default:
currentDocTypeOption = DocTypeOption.Regular;
}

const selected = Observable.create<DocTypeOption>(owner, currentDocTypeOption);

const doSetDocumentType = async () => {
let docType: DocumentType;
if (selected.get() === DocTypeOption.Regular) {
docType = DOCTYPE_NORMAL;
} else if (selected.get() === DocTypeOption.Template) {
docType = DOCTYPE_TEMPLATE;
} else {
docType = DOCTYPE_TUTORIAL;
}

const {trunkId} = docPageModel.currentDoc.get()!.idParts;
await docPageModel.appModel.api.persistType(docType, trunkId);
window.location.replace(urlState().makeUrl({
docPage: "settings",
fork: undefined, // will be automatically set once the page is reloaded
doc: trunkId,
}));
};

const docTypeOption = (
{
type,
label,
description,
itemTestId
}: {
type: DocTypeOption,
label: string | any,
description: string,
itemTestId: DomElementMethod | null
}) => {
return radioCheckboxOption(selected, type, dom('div',
dom('div',
dom('strong', label),
),
dom('div',
dom.style('margin-top', '8px'),
dom('span', description)
),
itemTestId,
));
};

const documentTypeOptions = () => [
cssRadioCheckboxOptions(
dom.style('max-width', '400px'),
docTypeOption({
type: DocTypeOption.Regular,
label: t('Regular document'),
description: t('Normal document behavior. All users work on the same copy of the document.'),
itemTestId: testId('doctype-modal-option-regular'),
}),
docTypeOption({
type: DocTypeOption.Template,
label: t('Template'),
description: t('Document automatically opens in {{fiddleModeDocUrl}}. ' +
'Anyone may edit, which will create a new unsaved copy.',
{
fiddleModeDocUrl: cssLink({href: commonUrls.helpAPI, target: '_blank'}, t('fiddle mode'))
}
),
itemTestId: testId('doctype-modal-option-template'),
}),
docTypeOption({
type: DocTypeOption.Tutorial,
label: t('Tutorial'),
description: t('Document automatically opens as a user-specific copy.'),
itemTestId: testId('doctype-modal-option-tutorial'),
}),
),
cssModalButtons(
bigBasicButton(t('Cancel'), dom.on('click', () => ctl.close()), testId('doctype-modal-cancel')),
bigPrimaryButton(t(`Confirm change`),
dom.on('click', doSetDocumentType),
testId('doctype-modal-confirm'),
),
)
];
return [
cssModalTitle(t(`Change document type`)),
documentTypeOptions(),
testId('doctype-modal'),
];
});
}

private async _doSetEngine(val: EngineCode|undefined) {
const docPageModel = this._gristDoc.docPageModel;
if (this._engine.get() !== val) {
Expand All @@ -298,8 +427,6 @@ export class DocSettingsPage extends Disposable {
}
}



function getApiConsoleLink(docPageModel: DocPageModel) {
const url = new URL(location.href);
url.pathname = '/apiconsole';
Expand Down Expand Up @@ -343,6 +470,39 @@ function buildLocaleSelect(
);
}

type DocumentTypeItem = ACSelectItem & {type?: string};

const typeList: DocumentTypeItem[] = [{
label: t('Regular'),
type: ''
}, {
label: t('Template'),
type: 'template'
}, {
label: t('Tutorial'),
type: 'tutorial'
}].map((el) => ({
...el,
value: el.label,
cleanText: el.label.trim().toLowerCase()
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling t top-level like this can be problematic. I'd suggest wrapping this in a function that returns the array.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello @georgegevoian,

For my own culture can you be more explicit on what can be problematic with using t() at top level?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's because the t function may not be initialized correctly (it needs to download a JSON file with the locales).


function displayCurrentType(
owner: IDisposableOwner,
type: Observable<DocumentType|null>,
) {
const typeObs = Computed.create(owner, use => {
const typeCode = use(type) ?? "";
const typeName = typeList.find(ty => ty.type === typeCode)?.label || typeCode;
return typeName;
});
return dom(
'div',
typeObs.get(),
hexaltation marked this conversation as resolved.
Show resolved Hide resolved
testId('doctype-value')
);
}

const cssContainer = styled('div', `
overflow-y: auto;
position: relative;
Expand Down Expand Up @@ -462,7 +622,7 @@ enum TimingModalPage {
/**
* Enum for the different options in the timing modal.
*/
enum Option {
enum TimingModalOption {
/**
* Start timing and immediately forces a reload of the document and waits for the
* document to be loaded, to show the results.
Expand All @@ -474,6 +634,15 @@ enum Option {
Adhoc,
}

/**
* Enum for the different options in the document type Modal.
*/
enum DocTypeOption {
Regular,
Template,
Tutorial,
}

// A version that is not underlined, and on hover mouse pointer indicates that copy is available
const cssCopyLink = styled(cssLink, `
word-wrap: break-word;
Expand Down Expand Up @@ -509,3 +678,13 @@ const cssWrap = styled('p', `
const cssRedText = styled('span', `
color: ${theme.errorText};
`);

const cssDocTypeContainer = styled('div', `
display: flex;
width: 172px;
align-items: center;
justify-content: space-between;
& > * {
display: inline-block;
}
`);
1 change: 1 addition & 0 deletions app/client/ui2018/cssVars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ export const theme = {

/* Checkboxes */
checkboxBg: new CustomProp('theme-checkbox-bg', undefined, colors.light),
checkboxSelectedFg: new CustomProp('theme-checkbox-selected-bg', undefined, colors.lightGreen),
checkboxDisabledBg: new CustomProp('theme-checkbox-disabled-bg', undefined, colors.darkGrey),
checkboxBorder: new CustomProp('theme-checkbox-border', undefined, colors.darkGrey),
checkboxBorderHover: new CustomProp('theme-checkbox-border-hover', undefined, colors.hover),
Expand Down
Loading
Loading