Summary: Cursor position observable was created using GrainJS, but the fields it was using were created using knockout observables. In a result the cursor position wasn't recomputed when a view was changed or an active section was deleted.
Test Plan: Browser tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2850
Test Plan:
Tested manually by enabling/disabling wrapping and changing alignment on reference columns. Existing test updated to check that
reference columns have cell formatting options available.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2853
Summary:
Bug summary: if in right bar user starts changing name of column, but then clicks on a different column name in table, THAT column will have its name changed.
This bug occurs because the save method is invoked by a blur event on a input field, which is triggered after all computed observables are calculated. Save method gets an observable to update, which by the time a blur event triggers, is changed to a new column.
The solution was to forcefully trigger the blur event as soon as possible - here by subscribing to the cursor position observable.
Test Plan: Browser tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2845
Test Plan: Block read access to column A based on the condition rec.B == 1. Then setting B = 1 in a row makes the cell under A grey.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: paulfitz, dsagal
Differential Revision: https://phab.getgrist.com/D2828
Summary:
A recently added stress test ("deletes documents reasonably quickly" in removedAt.ts) is sporadically failing under postgres. It looks like typeorm's .save() method is in some way unreliable when setting a table with multi-column primary keys, via a ManyToMany relation. This diff replaces the .save() with explicit inserts/deletes.
I modified _repairWorkspaceGuests recently, so thought that change might have been the problem. However under the stress test, failures occur as often in _repairOrgGuests (not changed recently) as in _repairWorkspaceGuests (changed recently).
For reference, see schema diagram at https://grist.quip.com/wWpRAMe058Nl/Home-DB (the table being updated is `group_users`).
Possibly related issue: https://github.com/typeorm/typeorm/issues/4122
Test Plan:
After this change, stress test runs well on postgres locally (no failure 70 iterations); before it would fail on postgres within 3 iterations typically.
Separately: I gave a test that failed a little more time to return, and confirmed it was no slower on average, so I think it was unrelated.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D2848
Summary:
The `_repairWorkspaceGuests` method is slow for workspaces with large numbers of documents. It makes a query that produces a lot of rows. The query itself is tolerable, but TypeORM processing uses enough CPU to be a likely culprit in some production instability. This diff splits the query into two pieces that are logically independent, but which when combined were resulting in the number of rows being the product of the two pieces. Once split, there is also a where clause that can be applied to one of the pieces.
The purpose of the method is to add every user that a document within a workspace is shared with to a "guest" group of the workspace itself. The design of "guest" groups is not ideal, but this diff leaves the design unchanged and is intended only to speed up operation.
Made some small tweaks to the timing of a flakey test, and temporarily recreated the `samples` directory removed in a previous diff (this is currently breaking tests badly on a fresh worker without a `samples` directory lying around)
Test Plan: added test; existing tests pass
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D2844
Summary:
- this is a core search code refactoring
- this diff should fix the js error that was happening when searching across pages.
Test Plan: Tested manually on dev's environment. Tests shows no regression and successfully fixes the js error.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2837
Summary:
This cleans up a few things about SELF_HYPERLINK urls:
* Use `urlId` rather than `docId`.
* Correctly merge personal org subdomain.
* In dev environment, use clearer port number.
Test Plan: updated test
Reviewers: alexmojaki, dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2841
Summary:
Names of private tables and columns were leaking via Code View.
This plugs that leak.
Test Plan: adds test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2840
Summary: Cells will remember their previous state when user pressed the escape key. Grist will offer a way to continue with the draft, by showing notification and a tooltip above the editor.
Test Plan: Browser tests were created
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2822
Summary:
- Sending `ESCAPE` while hovering the options panel was not working
- This is because of the keepExpanded flag which was set to true
- Solution is to set the keepExpanded flag to false, prior to toggling the Menu
Test Plan: - Tested manually
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2836
Test Plan: Set all columns to hidden and reloaded document.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2832
Summary:
We used tslint earlier, and on switching to eslint, some rules were not
transfered. This moves more rules over, for consistent conventions or helpful
warnings.
- Name private members with a leading underscore.
- Prefer interface over a type alias.
- Use consistent spacing around ':' in type annotations.
- Use consistent spacing around braces of code blocks.
- Use semicolons consistently at the ends of statements.
- Use braces around even one-liner blocks, like conditionals and loops.
- Warn about shadowed variables.
Test Plan: Fixed all new warnings. Should be no behavior changes in code.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2831
Test Plan: Created a user attribute under access rules, set the attribute to look up to user.LinkKey.e, confirmed that setting e_ in the URL modified access.
Reviewers: dsagal, paulfitz
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2824
Summary: Extract out function _onClickHyperlink
Test Plan: Made a table, formatted column as hyperlink, added values including a link to another page in the document, another document, and an external website, clicked on all the links and only the first one didn't open a new tab.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2826
Summary:
This diff implements the search improvement that are mentioned here https://grist.quip.com/j1biAmfGkbzV/Search-Improvements.
CAVEATS:
I've noticed a bit of a usability caveats: the tooltips overlap with the new `search all pages` checkbox, which requires user to move cursor away for a bit in order to be able to click the button.
{F28224}
I've experimented locally with tooltips showing on both sides of the arrows, but it overlaps with the cross icon so could also be an issue. I couldn't think of any clear simple alternative, probably not too big of an issue anyway.
Test Plan: Added new test.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2818
Summary: Editor position wasn't restored on a long list, where the rows haven't been shown yet (the scroll haven't happend yet).
Test Plan: Browser tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2823
Summary:
- The parameter was inadvertently removed by userOverrideParams().
- when passed a function to `urlState().setHref(...)` or `urlState().makeUrl(...)` it is important that the function does not mutate the state that it receives as argument.
Link to the related task: https://gristlabs.getgrist.com/doc/check-ins/p/5#a1.s9.r791.c19
Test Plan: Adds test of the persistence by slightly modifying existing nbrowser/AccessRules2 tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D2820
Summary:
For conversions between Choice and ChoiceList, it makes more sense to preserve
the list of choices than to re-parse it from data.
Reported by Anais. Creating Choices from parsing ChoiceList cell values was
particularly poor, resulting in choices like "L,Foo,Bar".
Test Plan: Added a test case
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2819
Summary: Grist document, when reloaded, is able to restore the latest cursor position and the editor state.
Test Plan: Browser test were created.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D2808
Summary: Text editor for Integer and Numeric column was showing null or undefined when the underlying value was null.
Test Plan: Browser test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2817
Summary:
Currently, to compute intermediate steps in a bundle, the bundle
is sent to the data engine to process. Then, if the intermediate
steps break a rule, it is reverted. One problem introduced by
checking permissions this late is that the data engine can be
exposed for formulas with python code by users who don't have the
right to change formulas. This diff pre-checks cases that change
formulas.
Test Plan: added a test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2816
Summary:
Auto-complete helps enter correct column names, and when incorrect ones are
entered, we now show an error and prevent saving the rules.
In an unrelated tweak, fix focusing of ACLFormula when clicking into scroll area.
Test Plan: Added a test case for showing invalid columns
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2815
Summary: For row creations and deletions, treat `rec` and `newRec` variables as identical. This simplifies writing a single rule that controls multiple permissions.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2812
Summary:
When scrolling quicly through a column with hyperlinks, null could be passed to
a function that didn't expect it. Added better types would help catch it.
Test Plan: Tested manually
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2813
Summary:
This diff discounts indirect changes for access control purposes. A UserAction that updates a cell A, which in turn causes changes in other dependent cells, will be considered a change to cell A for access control purposes.
The `engine.apply_user_actions` method now returns a `direct` array, with a boolean for each `stored` action, set to `true` if the action is attributed to the user or `false` if it is attributed to the engine. `GranularAccess` ignores actions attributed to the engine when checking for edit rights.
Subtleties:
* Removal of references to a removed row are considered direct changes.
* Doesn't play well with undos as yet. An action that indirectly modifies a cell the user doesn't have rights to may succeed, but it will not be reversible.
Test Plan: added tests, updated tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2806
Summary:
- Adds a new ChoiceList type, and widgets to view and edit it.
- Store in SQLite as a JSON string
- Support conversions between ChoiceList and other types
Test Plan: Added browser tests, and a test for how these values are stored
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2803
Summary: This treats newRec in the same way as rec in access formulas.
Test Plan: updated test for column renames; autocomplete checked manually.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2810
Summary: Saving sort or filter is not permitted in readonly mode. Hence we remove the button. The diff adds a new unit to
Test Plan: Adds test for behaviour to `nbrowser/ReadOnlyMode`. Also adds tests for other Save buttons related to sort & filter that should be disabled.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2804
Summary:
- close sort&filter menu when clicking Save/Revert buttons
- also closes when clicking Apply/Cancel from a nested filter menu
Test Plan:
- updated existing test to match new spec
- added new test to cover new behaviour
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2799
Summary: - Combination of styling of what's in the dropdown and what's in the sort config
Test Plan: adds new nbrowser test
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2798
Summary:
- Clicking quickly on the small save/revert button was caussing the
tooltip to stay around.
- But if user waited a little bit before clicking the save button,
the tooltip was shown, and then properly removed when the button was removed.
- Code was missing propertly handling of disposal before the tooltip
were shown.
Test Plan: Added test case to the projects/tooltip.ts tests
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2797
Summary:
- If you type into the "Everyone" / "Everyone Else" rule, and it stops being
the default rule, there will now be an extra row with a "+" button to add a
new default rule
- Switch to ACE-supported auto-resizing (for better scrollbars handling)
- Tweak ACE padding styles for better-looking scrolling.
Test Plan: Added a test case for the extra "+" button.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2796
Summary:
- Supports auto-completion
- Supports various keyboard interactions to delete/select/copy-paste tokens
- Supports various mouse interactions to select and move tokens.
- Supports undo/redo while editing tokens.
Test Plan: stand-alone fixture and browser test verify the supported behaviors
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2789
Summary:
- New sort and filter button has several states
- Empty / unsaved / saved
- offers small save/revert button when unsaved
- Fix little issue with hanging tooltip when the refElem is disposed.
- The problem was that if you hover the save (or revert) button
and then click the button, it causes the button to disappear,
but the tooltip was staying.
Test Plan: Updated all tests to match the new UI.
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: dsagal, paulfitz
Differential Revision: https://phab.getgrist.com/D2795
Summary: Select all functionality by clicking the upper left margin corner
Test Plan: Browser test created
Reviewers: dsagal, paulfitz
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2793
Summary:
When a document has an exception to allow copies,
unset that option on any copies of the document.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2794
Summary:
Exceptional document operations (particularly `system` and `nascent`
operations) should never be denied by a granular access rule.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2792
Summary: Access to structural tables currently depends on SchemaEdit permission. We now make an exception for owner access to _grist_ACLResources and _grist_ACLRules, giving them unconditional access. It was too easy for owners to lock themselves out of editing access rules.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2790
Summary:
- Update rules to be more like we've had with tslint
- Switch tsserver plugin to eslint (tsserver makes for a much faster way to lint in editors)
- Apply suggested auto-fixes
- Fix all lint errors and warnings in core/, app/, test/
Test Plan: Some behavior may change subtly (e.g. added missing awaits), relying on existing tests to catch problems.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2785
Summary:
update user.Link to user.LinkKey in autocomplete.
This was a late change on the backend.
Test Plan: checked manually
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2787
Summary:
attribute ActiveDoc log messages to users regardless of whether
they were triggered via a client or directly via api
Test Plan: log messages checked manually
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2786
Summary: add user and docId to heartbeat logging
Test Plan: checked manually
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2784
Summary:
This removes some unintentional repetition of work when there are
no row-level rules (there was a missing `return`).
Test Plan: existing tests pass
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2782
