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
