Summary:
First iteration for comments system for Grist.
- Comments are stored in a generic metatable `_grist_Cells`
- Each comment is connected to a particular cell (hence the generic name of the table)
- Access level works naturally for records stored in this table
-- User can add/read comments for cells he can see
-- User can't update/remove comments that he doesn't own, but he can delete them by removing cells (rows/columns)
-- Anonymous users can't see comments at all.
- Each comment can have replies (but replies can't have more replies)
Comments are hidden by default, they can be enabled by COMMENTS=true env variable.
Some things for follow-up
- Avatars, currently the user's profile image is not shown or retrieved from the server
- Virtual rendering for comments list in creator panel. Currently, there is a limit of 200 comments.
Test Plan: New and existing tests
Reviewers: georgegevoian, paulfitz
Reviewed By: georgegevoian
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3509
Summary:
There is a new column in users table called ref (user reference).
It holds user's unique reference number that can be used for features
that require some kind of ownership logic (like comments).
Test Plan: Updated tests
Reviewers: georgegevoian, paulfitz
Reviewed By: georgegevoian, paulfitz
Differential Revision: https://phab.getgrist.com/D3641
Summary:
Adds a "Duplicate Table" menu option to the tables listed on
the Raw Data page. Clicking it opens a dialog that allows you to
make a copy of the table (with or without its data).
Test Plan: Python, server, and browser tests.
Reviewers: jarek, paulfitz
Reviewed By: jarek, paulfitz
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3619
Summary:
This diff adds a preview of the value of certain autocomplete suggestions, especially of the form `$foo.bar` or `user.email`. The main initial motivation was to show the difference between `$Ref` and `$Ref.DisplayCol`, but the feature is more general.
The client now sends the row ID of the row being edited (along with the table and column IDs which were already sent) to the server to fetch autocomplete suggestions. The returned suggestions are now tuples `(suggestion, example_value)` where `example_value` is a string or null. The example value is simply obtained by evaluating (in a controlled way) the suggestion in the context of the given record and the current user. The string representation is similar to the standard `repr` but dates and datetimes are formatted, and the whole thing is truncated for efficiency.
The example values are shown in the autocomplete popup separated from the actual suggestion by a number of spaces calculated to:
1. Clearly separate the suggestion from the values
2. Left-align the example values in most cases
3. Avoid having so much space such that connecting suggestions and values becomes visually difficult.
The tokenization of the row is then tweaked to show the example in light grey to deemphasise it.
Main discussion where the above was decided: https://grist.slack.com/archives/CDHABLZJT/p1661795588100009
The diff also includes various other small improvements and fixes:
- The autocomplete popup is much wider to make room for long suggestions, particularly lookups, as pointed out in https://phab.getgrist.com/D3580#inline-41007. The wide popup is the reason a fancy solution was needed to position the example values. I didn't see a way to dynamically resize the popup based on suggestions, and it didn't seem like a good idea to try.
- The `grist` and `python` labels previously shown on the right are removed. They were not helpful (https://grist.slack.com/archives/CDHABLZJT/p1659697086155179) and would get in the way of the example values.
- Fixed a bug in our custom tokenization that caused function arguments to be weirdly truncated in the middle: https://grist.slack.com/archives/CDHABLZJT/p1661956353699169?thread_ts=1661953258.342739&cid=CDHABLZJT and https://grist.slack.com/archives/C069RUP71/p1659696778991339
- Hide suggestions involving helper columns like `$gristHelper_Display` or `Table.lookupRecords(gristHelper_Display=` (https://grist.slack.com/archives/CDHABLZJT/p1661953258342739). The former has been around for a while and seems to be a mistake. The fix is simply to use `is_visible_column` instead of `is_user_column`. Since the latter is not used anywhere else, and using it in the first place seems like a mistake more than anything else, I've also removed the function to prevent similar mistakes in the future.
- Don't suggest private columns as lookup arguments: https://grist.slack.com/archives/CDHABLZJT/p1662133416652499?thread_ts=1661795588.100009&cid=CDHABLZJT
- Only fetch fresh suggestions specifically after typing `lookupRecords(` or `lookupOne(` rather than just `(`, as this would needlessly hide function suggestions which could still be useful to see the arguments. However this only makes a difference when there are still multiple matching suggestions, otherwise Ace hides them anyway.
Test Plan: Extended and updated several Python and browser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3611
Summary:
This diff adds a new `BulkAddOrUpdateRecord` user action which is what is sounds like:
- A bulk version of the existing `AddOrUpdateRecord` action.
- Much more efficient for operating on many records than applying many individual actions.
- Column values are specified as maps from `colId` to arrays of values as usual.
- Produces bulk versions of `AddRecord` and `UpdateRecord` actions instead of many individual actions.
Examples of users wanting to use something like `AddOrUpdateRecord` with large numbers of records:
- https://grist.slack.com/archives/C0234CPPXPA/p1651789710290879
- https://grist.slack.com/archives/C0234CPPXPA/p1660743493480119
- https://grist.slack.com/archives/C0234CPPXPA/p1660333148491559
- https://grist.slack.com/archives/C0234CPPXPA/p1663069291726159
I tested what made many `AddOrUpdateRecord` actions slow in the first place. It was almost entirely due to producing many individual `AddRecord` user actions. About half of that time was for processing the resulting `AddRecord` doc actions. Lookups and updates were not a problem. With these changes, the slowness is gone.
The Python user action implementation is more complex but there are no surprises. The JS API now groups `records` based on the keys of `require` and `fields` so that `BulkAddOrUpdateRecord` can be applied to each group.
Test Plan: Update and extend Python and DocApi tests.
Reviewers: jarek, paulfitz
Reviewed By: jarek, paulfitz
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3642
Summary:
Fixes a bug noted here: https://grist.slack.com/archives/C069RUP71/p1662564341132349
This bug could happen quite easily as follows:
1. Have a formula in a summary table such as `$group.amount`. Typically there's also a `SUM` but that's not essential.
2. Find a group with nonzero values of `amount`.
3. Delete all rows in that group in the source table. Typically that just means one row in a lonely group.
4. The summary table row is automatically deleted.
5. Try to undo. This raises an error about trying to update a non-existent summary table row.
I tried to account for this undo problem in https://phab.getgrist.com/D3489 by not saving the updated value for `$group` when it was found to be empty. The reason this was insufficient is that `$group.amount` is immediately invalidated anyway when the source row(s) are deleted (I think because that's just how dependency relations involving references work) *and* the calculated value of `$group.amount` changes even if `$group` doesn't. For example, `$group.amount` may have previously been `[100, 200]`. After deleting the rows, `$group.amount` becomes `[0, 0]`. Keeping `$group` unchanged prevents `$group.amount` from just being `[]`, but deleting the source rows means that the amounts become the numeric default `0` which is still a change. This change in value is then noted which leads to saving an undo action to update the summary table record. All this happens in step 3 above, and the summary record is only deleted after that point.
This diff removes that special handling for `group` and instead adds a more general fix to `action_summary.py`. This inserts undo actions for deleted rows at the beginning of the undo list rather than at the end, which was already done for deleted tables and columns.
Test Plan: Python tests
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3626
Eg. before this commit, this table would result in Date columns:
| A | B |
| ----- | -- |
| FALSE | 0 |
For now, even FALSE is parsed as Numeric (not sure why we don't have
a BooleanConverter).
Summary:
This applies the set of dependabot suggestions that are currently
passing tests on grist-core. There are a lot more suggestions to
come, an unusual number are not passing tests because tests were
briefly broken.
The list of suggestions is extracted from:
https://api.github.com/repos/gristlabs/grist-core/pulls?search=status:success+state:open
And then applied using:
yarn upgrade package1@version1 package2@version2 ....
After application, any new entries in package.json are pruned, leaving
just updated entries and yarn.lock changes.
Non-trivial code updates include:
* A change related to axios typing
* A change related to jquery dropping `size()` in favor of `length`
Test Plan: existing tests should pass
Reviewers: jarek
Reviewed By: jarek
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3621
Summary:
Undo often leads to errors, especially with summary tables. One example is here: https://grist.slack.com/archives/C069RUP71/p1662564341132349
This diff simply decorates all relevant tests in 3 files testing summary tables with `@test_engine.test_undo`. This didn't catch any new bugs or reveal the problem in the thread above, but it seems good to have.
Test Plan: this
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3624
Summary:
Python 2 only needs to be supported for the sake of old documents and formulas. This doesn't apply to the separate sandboxes that parse files for imports. Using Python 3 only allows using newer libraries and library versions. In particular, the latest version of openpyxl doesn't support Python 2. This will also make it easier to make other similar changes in the future, such as replacing messytables with a modern library. See https://grist.slack.com/archives/C0234CPPXPA/p1661261829343999?thread_ts=1661260442.837959&cid=C0234CPPXPA
The latest openpyxl is better at handling a particular edge case with broken dates in Excel, but still doesn't quite do what we want, so we monkeypatch it. Discussion: https://grist.slack.com/archives/C02EGJ1FUCV/p1661440851911869?thread_ts=1661154219.515549&cid=C02EGJ1FUCV
Setting `preferredPythonVersion` to '3' in SafePythonComponent ensures that JS always creates import sandboxes that use Python 3. Within Python, a module used by all imports will raise an error in Python 2. Python unit tests of imports are now only run in Python 3, using the `load_tests` protocol of `unittest`.
Test Plan: Mostly existing tests. Added another strange date to the Excel fixture.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3606
Summary:
Based on https://github.com/gristlabs/grist-core/pull/251. It may not look like it, but there's very little going on in this diff:
- Tweak the DATEVALUE doctest for Python 2/3 compatibility.
- Mirrors the PR's changes to requirements3.txt in requirements.txt, i.e. make the same dependency upgrades in Python 2.
- Make the same upgrades in the thirdparty folder for the Python 2 nacl sandbox.
Test Plan: Updated one doctest for dateutil. Checked changelog of sortedcontainers. html5lib is only used by messytables and isn't actually relevant.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3609
Summary:
Raise an exception with a customised message for two cases when a user tries on operation directly on a table without `.all`:
1. For `Table.Col`, where `Col` is an existing column, suggest `Table.all.Col`. If `Col` doesn't exist as a column, fall back to the standard AttributeError.
2. When iterating directly over a table, e.g. `[r for r in Table]`, suggest looping over `Table.all` instead.
Test Plan: Added Python unit tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3593
Summary:
Makes the following improvements to formula autocomplete:
- When a user types `$RefCol` (or part of it), also show `$RefCol.VisibleCol` (replace actual column names) in the autocomplete even before the `.` is typed, to help users understand the difference between a raw reference/record and its visible column.
- When a user types a table name, show `.lookupOne` and `.lookupRecords` in the autocomplete, again even before the `.` is typed.
- For `.lookupRecords(` and `.lookupOne(`, once the `(` is entered, suggest each column name as a keyword argument.
- Also suggest lookup arguments involving compatible reference columns, especially 'reverse reference' lookups like `refcol=$id` which are very common and difficult for users.
- To support these features, the Ace editor autocomplete needs some patching to fetch fresh autocomplete options after typing `.` or `(`. This also improves unrelated behaviour that wasn't great before when one column name is contained in another. See the first added browser test.
Discussions:
- https://grist.slack.com/archives/CDHABLZJT/p1659707068383179
- https://grist.quip.com/HoSmAlvFax0j#MbTADAH5kgG
- https://grist.quip.com/HoSmAlvFax0j/Formula-Improvements#temp:C:MbT3649fe964a184e8dada9bbebb
Test Plan: Added Python and nbrowser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3580
Summary: Extend formula error messages with explanations from https://github.com/friendly-traceback/friendly-traceback. Only for Python 3.
Test Plan: Updated several Python tests. In general, these require separate branches for Python 2 and 3.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3542
Summary: Using the `random` module in the Grist `UUID()` function is not cryptographically secure, and is only necessary for the old pynbox (Python 2) sandbox which doesn't support `os.urandom`. This diff uses the `uuid.uuidv4()` function from the Python standard library when possible, which is more secure, only falling back to the old implementation when necessary.
Test Plan: Added Python unit tests to check both implementations.
Reviewers: dsagal
Subscribers: paulfitz, dsagal
Differential Revision: https://phab.getgrist.com/D3578
Summary:
Formulas in summary tables were being associated with the source table for automatic updating. When a table/column was renamed such that the formula needed to update to match, it would look for a column with the same colId but in the source table. Such a column might not exist which would lead to an error, or if it existed then the update would be wrong.
This association was created while building formulas to display in the code view in a nested `_Summary` class, it didn't need to exist at all. So this diff simply prevents the association from being created.
User report and discussion: https://grist.slack.com/archives/C0234CPPXPA/p1659717322297019
Test Plan: Extended `TestSummary.test_table_rename` Python test.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3568
Summary:
Conditional formatting can now be used for whole rows.
Related fix:
- Font styles weren't applicable for summary columns.
- Checkbox and slider weren't using colors properly
Test Plan: Existing and new tests
Reviewers: paulfitz, georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3547
Summary:
The MIN and MAX functions for formulas previously only considered numbers, ignoring other types, including dates. An example of this being a problem is here: https://community.getgrist.com/t/last-field-circularreferror-what-is-it/1114/4 . Using `MIN` on a column of dates would return 0 (the default) which gets converted to 1970-01-01. Users have to use `min` instead, which is confusing, and doesn't work when some values are empty.
This diff lets the functions operate on date and datetime values. A mixture of dates and datetimes is allowed, even though these cannot usually be compared in Python. Mixing dates and numbers will raise an exception.
Test Plan: Extended doctests
Reviewers: jarek, paulfitz
Reviewed By: jarek
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3560
Summary: Displays a live row count of each table on the Raw Data page.
Test Plan: Browser tests.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3540
Summary:
This calls a new `initialize` method on the sandbox before we start
doing calculations with it, to make sure that `random.seed()` has
been called. Otherwise, if the sandbox is cloned from a checkpoint,
the seed will have been reset.
The `initialize` method includes the functionality previously done
by `set_doc_url` since it is also initialization/personalization and
this way we avoid introducing another round trip to the sandbox.
Test Plan: tested with grist-core configured to use gvisor
Reviewers: georgegevoian, dsagal
Reviewed By: georgegevoian, dsagal
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3549
Summary:
Comprehensions iterating over `Table.all` like `[foo.bar for foo in Table.all]` led to an error when renaming the column `bar`. This diff fixes that so that renaming `bar` does the same thing as for a comprehension over `Table.lookupRecords()`. Note that `next(foo for foo in Table.all).bar` is still not supported, as the same is not supported for `Table.lookupRecords()` either.
Discussion: https://grist.slack.com/archives/C069RUP71/p1658360276762949
Test Plan: Parametrised existing Python test to test the same thing for both `all` and `lookupRecords`
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3538
Summary:
Changes auto-generated summary table IDs from e.g. `GristSummary_6_Table1` to `Table1_summary_A_B` (meaning `Table1` grouped by `A` and `B`). This makes it easier to write formulas involving summary tables, make API requests, understand logs, etc.
Because these don't encode the source table ID as reliably as before, `decode_summary_table_name` now uses the summary table schema info, not just the summary table ID. Specifically, it looks at the type of the `group` column, which is `RefList:<source table id>`.
Renaming a source table renames the summary table as before, and now renaming a groupby column renames the summary table as well.
Conflicting table names are resolved in the usual way by adding a number at the end, e.g. `Table1_summary_A_B2`. These summary tables are not automatically renamed when the disambiguation is no longer needed.
A new migration renames all summary tables to the new scheme, and updates formulas using summary tables with a simple regex.
Test Plan:
Updated many tests to use the new style of name.
Added new Python tests to for resolving conflicts when renaming source tables and groupby columns.
Added a test for the migration, including renames in formulas.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3508
Summary: Adds an InferenceTip which treats `Table.all` similarly to `Table.lookupRecords(...)`, so that `Table.all.foo` is changed to `Table.all.bar` when the column `foo` is renamed to `bar`.
Test Plan: Extended test for the `lookupRecords` case.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3521
Summary: Previously, changing the type of a column would clear its widget options and conditional style rules by default, with a few exceptions to explicitly keep them. This diff reverses that behaviour, keeping the options by default.
Test Plan: Updated several existing tests, plus lots of manual testing.
Reviewers: cyprien
Reviewed By: cyprien
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3491
Summary: When the `getSummarySourceGroup` function (used by the `$group` column) finds that the group is empty, raise a new special exception `EmptySummaryRow`. The engine catches this exception, avoids saving a value to the cell, and removes the record.
Test Plan: Updated several Python tests
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3489
Summary:
Summary tables now have their own raw viewsection, and are shown
under Raw Data Tables on the Raw Data page.
Test Plan: Browser and Python tests.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3495
Summary: When a formula raises an exception, we store that in the cell in memory. In Python 3, exceptions have a `__traceback__` attribute, which includes all the stack frames and local variables. This has huge memory leak potential. We already strategically format the exception when needed, we don't need to keep storing the actual traceback object.
Test Plan:
Manually tested that tracebacks are still sensible.
To check the effect on memory usage, made a simple test doc with 30k rows all containing an exception, and here's what ps aux says:
```
%MEM VSZ RSS
before: 2.4 681996 588828
after: 1.6 499052 405712
```
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3505
Summary:
- Upgrades to build-related packages:
- Upgrade typescript, related libraries and typings.
- Upgrade webpack, eslint; add tsc-watch, node-dev, eslint_d.
- Build organization changes:
- Build webpack from original typescript, transpiling only; with errors still
reported by a background tsc watching process.
- Typescript-related changes:
- Reduce imports of AWS dependencies (very noticeable speedup)
- Avoid auto-loading global @types
- Client code is now built with isolatedModules flag (for safe transpilation)
- Use allowJs to avoid copying JS files manually.
- Linting changes
- Enhance Arcanist ESLintLinter to run before/after commands, and set up to use eslint_d
- Update eslint config, and include .eslintignore to avoid linting generated files.
- Include a bunch of eslint-prompted and eslint-generated fixes
- Add no-unused-expression rule to eslint, and fix a few warnings about it
- Other items:
- Refactor cssInput to avoid circular dependency
- Remove a bit of unused code, libraries, dependencies
Test Plan: No behavior changes, all existing tests pass. There are 30 tests fewer reported because `test_gpath.py` was removed (it's been unused for years)
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3498
Summary:
Adds a Python function `REQUEST` which makes an HTTP GET request. Behind the scenes it:
- Raises a special exception to stop trying to evaluate the current cell and just keep the existing value.
- Notes the request arguments which will be returned by `apply_user_actions`.
- Makes the actual request in NodeJS, which sends back the raw response data in a new action `RespondToRequests` which reevaluates the cell(s) that made the request.
- Wraps the response data in a class which mimics the `Response` class of the `requests` library.
In certain cases, this asynchronous flow doesn't work and the sandbox will instead synchronously call an exported JS method:
- When reevaluating a single cell to get a formula error, the request is made synchronously.
- When a formula makes multiple requests, the earlier responses are retrieved synchronously from files which store responses as long as needed to complete evaluating formulas. See https://grist.slack.com/archives/CL1LQ8AT0/p1653399747810139
Test Plan: Added Python and nbrowser tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: paulfitz, dsagal
Differential Revision: https://phab.getgrist.com/D3429
Summary:
This addresses a rare bug where xls files with invalid dimensions
could not be imported into Grist due to how openpyxl handles
parsing them.
Test Plan: Server test.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3485
Summary:
Adds some special handling to summary table and lookup logic:
- Source rows with empty choicelists/reflists get a corresponding summary row with an empty string/reference when grouping by that column, instead of excluding them from any group
- Adds a new `QueryOperation` 'empty' in the client which is used in `LinkingState`, `QuerySet`, and `recursiveMoveToCursorPos` to match empty lists in source tables against falsy values in linked summary tables.
- Adds a new parameter `match_empty` to the Python `CONTAINS` function so that regular formulas can implement the same behaviour as summary tables. See https://grist.slack.com/archives/C0234CPPXPA/p1654030490932119
- Uses the new `match_empty` argument in the formula generated for the `group` column when detaching a summary table.
Test Plan: Updated and extended Python and nbrowser tests of summary tables grouped by choicelists to test for new behaviour with empty lists.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3471
Summary:
Adds a Python function `PEEK()` for use in formulas which temporarily sets a new attribute `Engine._peeking` which disables the `_use_node` method, preventing dependency tracking and allowing the given expression to use outdated values. This allows circumventing circular reference errors. It's particularly meant for trigger formulas although it works in normal formulas as well. The expression is wrapped in a `lambda` by `codebuilder` for lazy evaluation.
Discussion: https://grist.slack.com/archives/C0234CPPXPA/p1653571024031359
Test Plan: Added a Python unit test for circular trigger formulas using PEEK.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3453
Summary:
Fixing https://gristlabs.getgrist.com/doc/check-ins/p/12#a1.s19.r1045.c19 :
> Problem: user creates fresh new empty column. Users with access to write to that column, but not modify schema, will not in fact be able to write into it (since on first data entry column type needs to change). Experience is confusing.
Refactored `enter_indirection` and `leave_indirection` to a single context manager method for use with `with` instead of `try/finally`.
Used the new method in `_ensure_column_accepts_data` around column changing actions converting empty column to data column.
Test Plan:
Updated a Python test, reflecting that the correct actions are now marked as direct=False.
Tested manually that I can now add data to a blank column without schema access, while I wasn't able to before, and I still can't make other schema changes.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3446
Summary:
Importing a file with many columns would be very slow due to expensive calls to rebuild_usercode for each added column: https://grist.slack.com/archives/C02EGJ1FUCV/p1652395747972749?thread_ts=1652388644.394419&cid=C02EGJ1FUCV
This diff suppresses rebuild_usercode temporarily while adding columns in a loop in MakeImportTransformColumns, then calls it once afterwards.
Test Plan: Manually imported a wide file repeatedly. Eventually, whehn importing a file with 300 columns, generating the preview went from taking about 100 seconds to 20 seconds.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3445
Summary: To help with mistakes in formulas, forbid assigning to attributes of `rec` (e.g. `$foo = 1` which should probably be `==`) and ensure that there is at least one `return` in the formula (after maybe adding an implicit one at the end).
Test Plan: Extended Python unit test, updated tests which were missing return.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3439
Summary:
This allows limiting the memory available to documents in the sandbox when gvisor is used. If memory limit is exceeded, we offer to open doc in recovery mode. Recovery mode is tweaked to open docs with tables in "ondemand" mode, which will generally take less memory and allow for deleting rows.
The limit is on the size of the virtual address space available to the sandbox (`RLIMIT_AS`), which in practice appears to function as one would want, and is the only practical option. There is a documented `RLIMIT_RSS` limit to `specifies the limit (in bytes) of the process's resident set (the number of virtual pages resident in RAM)` but this is no longer enforced by the kernel (neither the host nor gvisor).
When the sandbox runs out of memory, there are many ways it can fail. This diff catches all the ones I saw, but there could be more.
Test Plan: added tests
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3398
Summary:
openpyxl was producing tuples while some older code expects lists. Choosing to convert the tuples to lists (instead of making the other code work with tuples) in case there's other similar issues still out there. Should fix the error mentioned in https://grist.slack.com/archives/C0234CPPXPA/p1652797247167719:
```
Traceback (most recent call last):
File "/gristroot/grist/sandbox/grist/sandbox.py", line 103, in run
ret = self._functions[fname](*args)
File "/gristroot/grist/sandbox/grist/imports/register.py", line 11, in parse_excel
return import_file(file_source)
File "/gristroot/grist/sandbox/grist/imports/import_xls.py", line 20, in import_file
parse_options, tables = parse_file(path)
File "/gristroot/grist/sandbox/grist/imports/import_xls.py", line 26, in parse_file
return parse_open_file(f)
File "/gristroot/grist/sandbox/grist/imports/import_xls.py", line 69, in parse_open_file
table_data_with_types = parse_data.get_table_data(rows, len(headers))
File "/gristroot/grist/sandbox/grist/parse_data.py", line 215, in get_table_data
row.extend([""] * missing_values)
AttributeError: 'tuple' object has no attribute 'extend'
```
Test Plan: Existing tests. Haven't figured out how to reproduce the original error.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3434
Summary:
Use openpyxl instead of messytables (which used xlrd internally) in import_xls.py.
Skip empty rows since excel files can easily contain huge numbers of them.
Drop support for xls files (which openpyxl doesn't support) in favour of the newer xlsx format.
Fix some details relating to python virtualenvs and dependencies, as Jenkins was failing to find new Python dependencies.
Test Plan: Mostly relying on existing tests. Updated various tests which referred to xls files instead of xlsx. Added a Python test for skipping empty rows.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3406
Summary:
- Better focus on the widget title
- Adding columns only to the current view section
- New popup with options when user wants to delete a page
- New dialog to enter table name
- New table as a widget doesn't create a separate page
- Removing a table doesn't remove the primary view
Test Plan: Updated and new tests
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3410
Summary: Adds a special user action `UpdateCurrentTime` which invalidates an internal engine dependency node that doesn't belong to any table but is 'used' by the `NOW()` function. Applies the action automatically every hour.
Test Plan: Added a Python test for the user action. Tested the interval periodically applying the action manually: {F43312}
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3389
Summary:
A new way for renaming tables.
- There is a new popup to rename section (where you can also rename the table)
- Renaming/Deleting page doesn't modify/delete the table.
- Renaming table can rename a page if the names match (and the page contains a section with that table).
- User can rename table in Raw Data UI in two ways - either on the listing or by using the section name popup
- As before, there is no way to change tableId - it is derived from a table name.
- When the section name is empty the table name is shown instead.
- White space for section name is allowed (to discuss) - so the user can just paste ' '.
- Empty name for a page is not allowed (but white space is).
- Some bugs related to deleting tables with attached summary tables (and with undoing this operation) were fixed (but not all of them yet).
Test Plan: Updated tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: georgegevoian
Differential Revision: https://phab.getgrist.com/D3360
Summary:
Summary columns now have their own conditional rules,
which are not shared with sister columns.
Test Plan: New test
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3388
Summary:
Updating one summary groupby columns is handle by the
`update_summary_section`. That routine is in charge of creating a new
summary table and transfer all possible columns from old table to the
new one, so that the new table appear to change only the minimum to
users.
The problem that this diff address is that, the logic to decide what columns we need to keep, only applies to the visible
fields and ignore the hidden columns, causing all hidden columns to be removed. This diff, simply
changes that routine to address all columns.
the mutliple fixes on the test_summary2.py result from a change of
ordering of columns: the culprit is the `group` columns, which is by
default a hidden columns and that had to be explicitely added back to
the new summary table. It is now transferred automatically, like other
columns, which does cause a little change of ordering on the db. This
should not result in any display order changes for the users.
Recent related diff: https://phab.getgrist.com/D3351
Test Plan: Includes new test case; both python and nbrowser
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3393
Summary:
- Previously showed "UnboundLocalError". Now will show:
Import failed: Failed to parse Excel file.
Error: No tables found (1 empty tables skipped)
- Also fix logging for import code
Test Plan: Added a test case
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3396
Summary: InitNewDoc is essentially only used to generate initialDocSql, so it doesn't make sense to set the timezone and locale. They are always set when actually creating a new doc anyway. Discussed in https://grist.slack.com/archives/C0234CPPXPA/p1650312714217089.
Test Plan: this
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3394
Summary:
Call ActiveDoc.removeUnusedAttachments every hour using setInterval, and in ActiveDoc.shutdown (which also clears said interval).
Unrelated: small fix to my webhooks code which was creating a redis client on shutdown just to quit it.
Test Plan:
Tweaked DocApi test to remove expired attachments by force-reloading the doc, so that it removes them during shutdown. Extracted a new testing endpoint /verifyFiles to support this test (previously running that code only happened with `/removeUnused?verifyfiles=1`).
Tested the setInterval part manually.
Reviewers: paulfitz, dsagal
Reviewed By: paulfitz
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3387
Summary:
Redesigning color picker:
- Single color palette (no light/dark switch)
- Ability to remove color (new empty button)
New font options in the color picker.
Font options are available on:
- Default cell style
- Conditional rules styles
- Choice/ChoiceList editor and token field
- Filters for Choice/ChoiceList columns
Design document:
https://www.figma.com/file/bRTsb47VIOVBfJPj0qF3C9/Grist-Updates?node-id=415%3A8135
Test Plan: new and updated tests
Reviewers: georgegevoian, alexmojaki
Reviewed By: georgegevoian, alexmojaki
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3335
Summary:
Description of the problem can be found here: https://grist.slack.com/archives/C069RUP71/p1634899282005600
- users removing a group by column that is of type numeric was
resulting in the column missing from the summary table. Where
instead is should be present as a 'SUM($group.${col.colId})'
formula column
- this diff fixes that issue and adds unit test
Test Plan: Should not break anything. Adds not test case.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3351
Summary: Adds a migration in preparation for future work on tracking and deleting attachments. This includes a `_grist_Attachments.timeDeleted` column which isn't used yet, and changing the storage format of user columns of type `Attachments`. DocStorage now treats Attachments like RefList in general (since they use JSON), which also prompted a tiny bit of refactoring.
Test Plan: Added a migration test case showing the change in format.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3352
Summary: While `$ref.other_ref` returns a reference (Record) allowing chaining more properties like `$ref.other_ref.foo`, reflists (RecordSet) did not allow this, e.g. `$reflist.other_ref` returned a plain list of records, preventing chaining more dot notation. Discussed here: https://grist.slack.com/archives/CDHABLZJT/p1648845745765839
Test Plan: Added a Python unit test. Formulas like `$reflist.other_ref` were already very common though, and getting the functionality code slightly wrong leads to a flood of test failures.
Reviewers: jarek
Reviewed By: jarek
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3354
Summary:
Adds a `data-grist-col-ref` attribute to the copied HTML, then uses that when pasting to look up the source column and retrieve info about it. Copies the info into the target column if:
- The document is the same (the docId hash matches)
- The source column still exists and has the same type as when copied
- The source type isn't Text, because in that case it's nice if type guessing still happens
- The target column is empty, meaning it has type Any (we check earlier that it's not a formula column)
The info copied is the type, widgetOptions, and reference column settings (visible and display columns) but not conditional formatting.
The changes are mostly in a function `parsePasteForView` which is based on `BaseView._parsePasteForView` but ported to TypeScript in a new file `BaseView2.ts`.
Added a useraction `MaybeCopyDisplayFormula` exposing an existing Python function `maybe_copy_display_formula` because the target column needs a slightly different display formula.
Test Plan: Added a new nbrowser test file and fixture doc.
Reviewers: cyprien
Reviewed By: cyprien
Subscribers: jarek, dsagal
Differential Revision: https://phab.getgrist.com/D3344
Summary:
JSON import logic was creating columns of type Int when JSON contained integral
values. This causes errors with large errors (e.g. millisecond timestamps), and
Numeric is generally the more convenient and common default.
Test Plan: TBD
Reviewers: jarek, alexmojaki
Reviewed By: jarek, alexmojaki
Subscribers: jarek, alexmojaki
Differential Revision: https://phab.getgrist.com/D3339
Summary: This changes Table.sample_record from a regular attribute to a property that's only computed when it's needed, which is only for autocompletion. This means it's not cached any more, but it's also not recomputed every time the schema changes. Profiling showed that _make_sample_record took a signification portion of time, and this change makes the tests 2 or 3 seconds faster.
Test Plan: existing tests
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3334
Summary:
Adding conditional formatting rules feature.
Each column can have multiple styling rules which are applied in order
when evaluated to a truthy value.
- The creator panel has a new section: Cell Style
- New user action AddEmptyRule for adding an empty rule
- New columns in _grist_Table_columns and fields
A new color picker will be introduced in a follow-up diff (as it is also
used in choice/choice list/filters).
Design document:
https://grist.quip.com/FVzfAgoO5xOF/Conditional-Formatting-Implementation-Design
Test Plan: new tests
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3282
Summary:
This is a fix for a bug discussed in https://grist.slack.com/archives/C069RUP71/p1645138610722889
I still haven't completely wrapped my head around it or figured out how to make a simple reproducible example, but the problem seems to be that a lookup can happen before the column(s) being looked up (the summary helper column in this case) have been computed fully (I think it got interrupted halfway by an OrderError). `do_lookup` would check via `engine._use_node` that the row IDs it found had all been computed already, but there might still be other rows that hadn't been computed yet and would also have values matching the lookup key, so it missed those.
This diff instead calls `_use_node` with no `row_ids` argument, which should ensure that all rows have already been computed.
At first I was worried about how this would affect performance, which led me down an optimisation rabbit hole, hence a bit of unrelated cleanup here and also https://phab.getgrist.com/D3310 . But it doesn't seem to be a problem, and IIUC it should actually make things better, although this code is pretty confusing.
Test Plan: Tested manually that the doc no longer behaves weirdly
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal, paulfitz
Differential Revision: https://phab.getgrist.com/D3308
Summary:
This is an attempt to optimise Engine._use_node. It doesn't seem to actually improve overall performance significantly, but it shouldn't make it worse, and I think it's an improvement to the code.
It turns out that there's no need to track a stack of compute frames any more. The only time we get close to nested evaluation, we set allow_evaluation=False to prevent it actually happening. So there's only one 'frame' during actual evaluation, which means we can get rid of the concept of frames entirely. This allows simplifying the code and letting the computer do less work in general.
Test Plan: this
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3310
Summary: I ran the python tests through a profiler and found that just generating the autocomplete context was a major cost, and this would happen with every schema change. By only generating it when needed for autocomplete, the time for tests reduced from ~38s to ~33s.
Test Plan: this
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3312
Summary:
- Removed string parsing and some type guessing code from parse_data.py. That logic is now implicitly done by ValueGuesser by leaving the initial column type as Any. parse_data.py mostly comes into play when importing files (e.g. Excel) containing values that already have types, i.e. numbers and dates.
- 0s and 1s are treated as numbers instead of booleans to keep imports lossless.
- Removed dateguess.py and test_dateguess.py.
- Changed what `guessDateFormat` does when multiple date formats work equally well for the given data, in order to be consistent with the old dateguess.py.
- Columns containing numbers are now always imported as Numeric, never Int.
- Removed `NullIfEmptyParser` because it was interfering with the new system. Its purpose was to avoid pointlessly changing a column from Any to Text when no actual data was inserted. A different solution to that problem was already added to `_ensure_column_accepts_data` in the data engine in a recent related diff.
Test Plan:
- Added 2 `nbrowser/Importer2` tests.
- Updated various existing tests.
- Extended testing of `guessDateFormat`. Added `guessDateFormats` to show how ambiguous dates are handled internally.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3302
Summary:
Adds `common/ValueGuesser.ts` with logic for guessing column type and widget options (only for dates/datetimes) from an array of strings, and converting the strings to the guessed type in a lossless manner, so that converting back to Text gives the original values.
Changes `_ensure_column_accepts_data` in Python to call an exported JS method using the new logic where possible.
Test Plan: Added `test/common/ValueGuesser.ts` to unit test the core guessing logic and a DocApi end-to-end test for what happens to new columns.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3290
Summary:
This makes a `user.SessionID` value available in information about the user, for use with trigger formulas and granular access rules. The ID should be constant within a browser session for anonymous user. For logged in users it simply reflects their user id.
This ID makes it possible to write access rules and trigger formulas that allow different anonymous users to create, view, and edit their own records in a document.
For example, you could have a brain-storming document for puns, and allow anyone to add to it (without logging in), letting people edit their own records, but not showing the records to others until they are approved by a moderator. Without something like this, we could only let anonymous people add one field of a record, and not have a secure way to let them edit that field or others in the same record.
Also adds a `user.IsLoggedIn` flag in passing.
Test Plan: Added a test, updated tests. The test added is a mini-moderation doc, don't use it for real because it allows users to edit their entries after a moderator has approved them.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3273
Summary:
Adds a method Table._num_rows using an empty lookup map column.
Adds a method Engine.count_rows which adds them all up.
Returns the count after applying user actions to be logged by ActiveDoc.
Test Plan: Added a unit test in Python. Tested log message manually.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3275
Summary:
Prevent most updates or removals of raw view sections and their fields in `useractions.py`. Only a fiew columns are allowed to be updated.
Removed the unused method `_UpdateViews` while I was at it.
Test Plan:
Added a Python test.
Tested manually that I can still make all expected changes, i.e. those allowed by the UI, e.g. reordering columns.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3263
Summary:
When possible, the original column headers from imported
files will now be used as the labels for Grist columns. This includes
values that were previously invalid Grist column identifiers, such
as those containing Unicode.
Test Plan: Updated server and browser tests.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3261
Summary:
As designed in https://grist.quip.com/fZSrAnJKgO5j/Add-or-Update-Records-API
Current `POST /records` adds records, and `PATCH /records` updates them by row ID. This adds `PUT /records` to 'upsert' records, applying the AddOrUpdate user action. PUT was chosen because it's idempotent. Using a separate method (instead of inferring based on the request body) also cleanly separates validation, documentation, etc.
The name `require` for the new property was suggested by Paul because `where` isn't very clear when adding records.
Test Plan: New DocApi tests
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3251
Summary: This is https://phab.getgrist.com/D3205 plus some changes (https://github.com/dsagal/grist/compare/type-convert...type-convert-server?expand=1) that move the conversion process to the backend. A new user action ConvertFromColumn uses `call_external` so that the data engine can delegate back to ActiveDoc. Code for creating formatters and parsers is significantly refactored so that most of the logic is in `common` and can be used in different ways.
Test Plan: The original diff adds plenty of tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3240
Summary:
New user action as described in https://grist.quip.com/fZSrAnJKgO5j/Add-or-Update-Records-API, with options to allow most of the mentioned possible behaviours.
The Python code is due to Alex (as should be obvious from the u in behaviours).
Test Plan: Added a unit test.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3239
Summary: This is the first step towards raw data views, merely adding metadata without any UI. Every 'normal' table now has a widget referenced by `rawViewSectionRef`. It has no parent view/page and cannot actually be viewed for now. The widget is created during the AddTable user action, and the migration creates a widget for existing tables.
Test Plan: Many tests had to be updated, especially tests that listed all view sections and/or fields.
Reviewers: jarek, dsagal
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3232
Summary:
Updating filters when user renames labels in a choice/choice list column.
When there are unsaved filters they are reverted to orginal values (only
for the affected column).
Test Plan: new tests
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3230
Summary:
Improving experience when editing group-by column:
- Disable column rename
- Allow changing most widget properties:
- Color/Background
- Number format
- Date/DateTime format (but not the timezone)
- All toggle options (for toggle column)
- Remove Edit button on Choice Edit
- Changing the underlying column should reset all those options back to the original column.
Test Plan: nbrowser
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3216
Summary:
- Improve readability of documentation of CONTAINS.
- Add leading underscore to Record._get_encodable_row_ids() to hide from
public docs, and avoid interfering with user fields.
- Fix up lint errors
Test Plan: No behavior changes
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3209
Summary:
Addresses several issues:
- Error 'Cannot modify summary group-by column' when changing Text ->
ChoiceList in the presence of summary tables.
- Error 'ModifyColumn in unexpected position' when changing ChoiceList -> Text
in the presence of summary tables.
- Double-evaluation of trigger formulas in some cases.
Fixes include:
- Fixed verification that summary group-by columns match the underlying ones,
and added comments to explain.
- Avoid updating non-metadata lookups after each doc-action (early lookups
generated extra actions to populate summary tables, causing the 'ModifyColumn
in unexpected position' bug)
- When updating formulas, do update lookups first.
- Made a client-side tweak to avoid a JS error in case of some undos.
Solution to reduce lookups is based on https://phab.getgrist.com/D3069?vs=on&id=12445,
and tests for double-evaluation of trigger formulas are taken from there.
Add a new test case to protect against bugs caused by incorrect order of
evaluating #lookup columns.
Enhanced ChoiceList browser test to check a conversion scenario in the presence
of summary tables, previously triggering bugs.
Test Plan: Various tests added or enhanced.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3184
