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:
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
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: 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:
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:
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: 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: 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: 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:
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:
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:
- 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: title
Test Plan: Added python unit test. Seems like the first test of sort_by in the whole codebase.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3124
Summary:
This unsets the `direct` flag for actions emitted when summary tables are updated. That means those actions will be ignored for access control purposes. So if a user has the right to change a source table, the resulting changes to the summary won't result in the overall action bundle being forbidden.
I don't think I've actually seen the use case that inspired this issue being filed. I could imagine perhaps a user forbidden from creating rows globally making permitted updates that could add rows in a summary (and it being desirable to allow that).
Test Plan: added tests
Reviewers: jarek
Reviewed By: jarek
Subscribers: dsagal, alexmojaki, jarek
Differential Revision: https://phab.getgrist.com/D3022
Summary:
Replacing a column leads to an unnecessary recalculation, and was happening on
every schema change. This is particularly noticeble for large docs, especially
for imports when each column's addition is a schema change in itself, so
recalculation of summary table groupings were happening many times.
Test Plan: Existing tests should pass. No tests yet for avoiding recalculation, but would be nice!
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3005
Summary: Just return a list from _get_col_subset, remove ColumnView class
Test Plan: none
Reviewers: dsagal, georgegevoian
Reviewed By: dsagal, georgegevoian
Subscribers: georgegevoian
Differential Revision: https://phab.getgrist.com/D2975
Summary: Updates the summary column type correctly, rebuilds the table model
Test Plan: Added a python unit test, tested manually in browser
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2973
Summary:
This makes it possible to set the type of a column to ReferenceList, but the UI is terrible
ReferenceList.ts is a mishmash of ChoiceList and Reference that sort of works but something about the CSS is clearly broken
ReferenceListEditor is just a text editor, you have to type in a JSON array of row IDs. Ignore the value that's present when you start editing. I can maybe try mashing together ReferenceEditor and ChoiceListEditor but it doesn't seem wise.
I think @georgegevoian should take over here. Reviewing the diff as it is to check for obvious issues is probably good but I don't think it's worth trying to land/merge anything.
Test Plan: none
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: georgegevoian
Differential Revision: https://phab.getgrist.com/D2914
Summary: Having CONTAINS be a class is a pain, undoing that mistake now
Test Plan: none needed
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2929
Summary:
Added CONTAINS 'function' which can be used in lookups
Changed LookupMapColumn._row_key_map to use right=set so one row can have many keys when CONTAINS is used.
Use CONTAINS to implement group column in summary table, while helper column in source table can reference and create multiple rows in summary table, especially when summarising by ChoiceList columns.
Use itertools.product to generate all combinations of lookup keys and groupby values.
cleanup
Test Plan: Added python unit tests.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: paulfitz, dsagal
Differential Revision: https://phab.getgrist.com/D2900
Summary:
Dealing with some things that bothered and sometimes confused me:
Make Table.Record[Set] provide the table argument automatically
Remove the classes from UserTable because they're not used anywhere and the Table/UserTable distinction is already confusing. They're not documented for users and they don't show up in autocomplete.
Remove RecordSet.Record because it was confusing me where that attribute was being set, but also this means .Record will work properly for users with columns named 'Record'.
Test Plan: existing tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2913
Summary: Changes that move towards python 3 compatibility that are easy to review without much thought
Test Plan: The tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2873
Summary:
- Introduce a new SQLiteDB migration, which adds DB columns for formula columns
- Newly added columns have the special ['P'] (pending) value in them
(in order to show the usual "Loading..." on the first load that triggers the migration)
- Calculated values are added to .stored/.undo fields of user actions.
- Various changes made in the sandbox to include .stored/.undo in the right order.
- OnDemand tables ignore stored formula columns, replacing them with special SQL as before
- In particular, converting to OnDemand table leaves stale values in those
columns, we should maybe clean those out.
Some tweaks on the side:
- Allow overriding chai assertion truncateThreshold with CHAI_TRUNCATE_THRESHOLD
- Rebuild python automatically in watch mode
Test Plan: Fixed various tests, updated some fixtures. Many python tests that check actions needed adjustments because actions moved from .stored to .undo. Some checks added to catch situations previously only caught in browser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2645
Summary:
this moves sandbox/grist to core, and adds a requirements.txt
file for reconstructing the content of sandbox/thirdparty.
Test Plan:
existing tests pass.
Tested core functionality manually. Tested docker build manually.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2563
