Summary:
Finishing imports now occurs in Node instead of the
data engine, which makes it possible to import into
on-demand tables. Merging code was also refactored
and now uses a SQL query to diff source and destination
tables in order to determine what to update or add.
Also fixes a bug where incremental imports involving
Excel files with multiple sheets would fail due to the UI
not serializing merge options correctly.
Test Plan: Browser tests.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3046
Test Plan: Only tested manually that path is included.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3056
Summary:
Make _rename_cell_choice return None for unchanged values
The tests actually passs without the implementation changes, because trim_update_action removed the noop updates. So I'm not sure if this is an improvement. It certainly seems that it would be slower in cases where every record is updated, and it's hard to say if it would be better in other cases.
Test Plan:
Checked doc actions in existing test.
Also tested for invalid existing values.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3052
Summary:
This makes the `core` test operate on a directory outside the
jenkins workspace, so that packages in the workspace don't
interfere with the test and obscure errors.
It also includes a small type fix for the `core` build.
Test Plan: updating a test
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3054
Summary:
- Update cookie module, to support modern sameSite settings
- Add a new cookie, grist_sid_status with less-sensitive value, to let less-trusted subdomains know if user is signed in
- The new cookie is kept in-sync with the session cookie.
- For a user signed in once, allow auto-signin is appropriate.
- For a user signed in with multiple accounts, show a page to select which account to use.
- Move css stylings for rendering users to a separate module.
Test Plan: Added a test case with a simulated Discourse page to test redirects and account-selection page.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3047
Summary:
With this diff, when a user opens a Grist document in a browser, they will be able to view its contents without waiting for the data engine to start up. Once the data engine starts, it will run a calculation and send any updates made. Changes to the document will be blocked until the engine is started and the initial calculation is complete.
The increase in responsiveness is useful in its own right, and also reduces the impact of an extra startup time in a candidate next-generation sandbox.
A small unrelated fix is included for `core/package.json`, to catch up with a recent change to `package.json`.
A small `./build schema` convenience is added to just rebuild the typescript schema file.
Test Plan: added test; existing tests pass - small fixes needed in some cases because of new timing
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3036
Summary:
New environmental variable GOOGLE_DRIVE_SCOPE that modifies the scope
requested for Google Drive integration.
For prod it has value https://www.googleapis.com/auth/drive.file which leaves
current behavior (Grist is allowed only to access public files and for private
files - it fallbacks to Picker).
For staging it has value https://www.googleapis.com/auth/drive.readonly which
allows Grist to access all private files, and fallbacks to Picker only when the file is
neither public nor private).
Default value is https://www.googleapis.com/auth/drive.file
Test Plan: manual and existing tests
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3038
Summary:
Bug reported by user: https://gristlabs.getgrist.com/doc/check-ins/p/3#a1.s7.r1183.c19p
Setting x axis to a column of type ChoiceList was breaking chart.
This diff fixes that by splitting the record into several records: one for each choice.
`test/nbrowser/ChartView1.ts` was becoming too big and long to run, so this diff introduces `test/nbrowser/ChartView2.ts` to add more test and `test/nbrowser/chartViewTestUtils.ts` to put all utilities or testing charts.
Test Plan: Adds new test.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3041
Summary:
["RenameChoices", table_id, col_id, renames]
Updates the data in a Choice/ChoiceList column to reflect the new choice names.
`renames` should be a dict of `{old_choice_name: new_choice_name}`.
This doesn't touch the choices configuration in widgetOptions, that must be done separately.
Frontend to be done in another diff.
Test Plan: Added two Python unit tests.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3050
Summary:
This adds a `user:delete` target to the `cli.sh` tool. The desired user will be deleted from our database, from sendgrid, and from cognito.
There is code for scrubbing the user from team sites, but it isn't yet activated, I'm leaving finalizing and writing tests for it for follow-up.
Test Plan: tested manually
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3043
Summary:
This removes the need for any information drawn from the content of recent actions when loading a document.
The undo/redo system does need some facts about recent actions up front. But that system has an important restriction: only actions a particular client is known to have generated can be undone by that client.
So in this diff, as we store which client has performed an action, we also store the few pieces of metadata about that action that the undo/redo system needs: `linkId`, `otherId`, `rowIdHint`, `isUndo` fields. These are all small integers (or in one case a boolean).
An existing limitation is that information about which client has performed which action is stored in memory in the worker, and not persisted anywhere. This diff does not change that limitation, meaning that undos continue to not survive a worker transition. A reasonable way to deal with that would be to back the store with redis.
Test Plan: existing tests pass
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3044
Summary: There was a bad regex processing the document url passed to the sandbox.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3048
Summary:
Tests DocApi endpoints _subscribe and _unsubscribe, including various bad inputs.
Tests that webhooks are sent to a test express server, with retrying on failure, filtered by event type, and waiting for isReadyColumn.
Test Plan: this
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3042
Summary:
Traceback is available on the Creator Panel in the formula editor. It is evaluated the same way as for normal formulas.
In case when the traceback is not available, only the error name is displayed with information that traceback is not available.
Cell with an error, when edited, shows the previous valid value that was used before the error happened (or None for new rows).
Value is stored inside the RaisedException object that is stored in a cell.
Test Plan: Created tests
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: alexmojaki, dsagal
Differential Revision: https://phab.getgrist.com/D3033
Summary: Adding more locale codes to support more countries in document settings
Test Plan: existing tests
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3018
Summary: Update _create_syntax_error_code to raise an error with similar arguments to the real arguments it already has, with our modifications.
Test Plan: Updated python unit tests
Reviewers: jarek, dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3040
Summary:
- Anchor links with row of 'new' could be created but weren't parsed or used
correctly. This fixes it.
- Also adds UIRowId type for row IDs which includes the special 'new' row. It's
already been used in places as `number|'new'`, this diff gives it a name usable in app/common
(it doesn't touch another name, RowId, that's been available in app/client).
Test Plan: Added a test assert for anchor links to new row
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3039
Summary:
See https://grist.quip.com/VKd3ASF99ezD/Outgoing-Webhooks
- 2 new DocApi endpoints: _subscribe and _unsubscribe, not meant to be user friendly or publicly documented. _unsubscribe should be given the response from _subscribe in the body, e.g:
```
$ curl -X POST -H "Authorization: Bearer 8fd4dc59ecb05ab29ae5a183c03101319b8e6ca9" "http://localhost:8080/api/docs/6WYa23FqWxGNe3AR6DLjCJ/tables/Table2/_subscribe" -H "Content-type: application/json" -d '{"url": "https://webhook.site/a916b526-8afc-46e6-aa8f-a625d0d83ec3", "eventTypes": ["add"], "isReadyColumn": "C"}'
{"unsubscribeKey":"3246f158-55b5-4fc7-baa5-093b75ffa86c","triggerId":2,"webhookId":"853b4bfa-9d39-4639-aa33-7d45354903c0"}
$ curl -X POST -H "Authorization: Bearer 8fd4dc59ecb05ab29ae5a183c03101319b8e6ca9" "http://localhost:8080/api/docs/6WYa23FqWxGNe3AR6DLjCJ/tables/Table2/_unsubscribe" -H "Content-type: application/json" -d '{"unsubscribeKey":"3246f158-55b5-4fc7-baa5-093b75ffa86c","triggerId":2,"webhookId":"853b4bfa-9d39-4639-aa33-7d45354903c0"}'
{"success":true}
```
- New DB entity Secret to hold the webhook URL and unsubscribe key
- New document metatable _grist_Triggers subscribes to table changes and points to a secret to use for a webhook
- New file Triggers.ts processes action summaries and uses the two new tables to send webhooks.
- Also went on a bit of a diversion and made a typesafe subclass of TableData for metatables.
I think this is essentially good enough for a first diff, to keep the diffs manageable and to talk about the overall structure. Future diffs can add tests and more robustness using redis etc. After this diff I can also start building the Zapier integration privately.
Test Plan: Tested manually: see curl commands in summary for an example. Payloads can be seen in https://webhook.site/#!/a916b526-8afc-46e6-aa8f-a625d0d83ec3/0b9fe335-33f7-49fe-b90b-2db5ba53382d/1 . Great site for testing webhooks btw.
Reviewers: dsagal, paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3019
Summary: Update LinkingState._makeSrcCellGetter to account for 'new'
Test Plan: Extended test in RightPanelSelectBy.ts
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3037
Summary:
Used to throw js errors: `Resize must be passed a displayed plot div element.`
Summarizing (or unsummarizing) causes the ChartView view instance to
be replace by a new one in the view layout.
However, the problem is that the old view instance get disposed only
after the new view instance is added to the view layout.
This causes the old view layout to try to resize chart while chart dom
has been removed from the dom (which Plotly does not support).
This diff fixes it by checking the the chart dom elemnt is still in
the dom before making the plotly call to resize the chart.
TODO: It feels weird that the old view instance gets disposed after
the new one get added. Maybe we should check that also.
Test Plan: New test added.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3035
Test Plan: Added a test with lots of time-parsing cases.
Reviewers: jarek
Reviewed By: jarek
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3034
Summary: Constructs a ClientQuery in a similar way to LinkingState to handle lists.
Test Plan: Extended SelectBySummary and SelectByRefList tests.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3030
Summary:
This is preparatory work for running tests with the new sandbox in jenkins.
* Makes a base image that is now shared by grist servers and jenkins workers.
* Needed to allow jenkins to run `sudo runsc`.
* Converged on port 2020 for ssh to workers and servers.
Test Plan: added one runsc-based test and confirmed it was run
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3029
Summary:
Python isdigit() returns true for unicode characters such as "²", which fail
when used as an argument to int().
Instead, be explicit about only considering characters 0-9 to be digits.
Test Plan: Added a test case which produces an error without this change.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3027
Summary:
While switching the destination site in the Duplicate Document dialog, there
were times when it was saveable even though destination workspaces were still
being fetched. This sometimes causes a test failure, with the document getting
saved to a workspace from the previously-selected org.
Test Plan:
Tested manually; reproduced by adding a conditional artificial delay
in _updateWorkspaces helper.
Reviewers: paulfitz, georgegevoian
Reviewed By: paulfitz, georgegevoian
Differential Revision: https://phab.getgrist.com/D3032
Summary:
Also fixes issue with group data options when switching to pie chart.
Issue was that if the group data picker was on, switching to the pie
chart was not hiding it.
Test Plan: Adds more tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3028
Summary:
After updating the jenkins test workers, chrome and python changes resulting in a scattering of test failures.
* Clicking on an icon that has been transformed to mirror around the y axis fails via selenium. Not sure why. "Fixed" by asking the browser to do the click.
* There was a small change from python 3.9.6 to python 3.9.7 that affected completion of property attributes.
Worker updates here: https://phab.getgrist.com/D3029
Test Plan: these tests
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3031
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:
This change prevents dragging tokens when the cursor is over the
delete button. Now, trying to drag after mousedown over the delete
button will do nothing if the cursor is released outside the delete
button, and will delete if the cursor is released inside the delete
button.
Test Plan: Tested manually.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3024
Summary:
A user without SchemaEdit permission was able to reorder pages, since
this changes _grist_Pages, and that table was left under control of
regular access rules. This diff tightens things up, to require
SchemaEdit for all metadata edits. The one remaining exception is
_grist_Attachments, which needs some reworking to play well with
granular access.
Test Plan: extended test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3025
Summary:
This is a follow up diff for https://phab.getgrist.com/D3021. Y-axis
draggable list used to blink when user changed either one of the x
axis or groupdata column.
This was due to the fact that all of theses axis are stored into the
same array and changing one of them changes the whole array even
though items relative to the y-axis actually were not changing.
This diff addresses this issue by 1) being carefull at not updating
the array of items when the changes do not impact y axis. And 2) by
adding a freeze observable allowing to freeze the draggable list of
y-axis while actions are being treated on the server.
Test Plan:
Catching such bug is hard, and given that it's only look and fill, maybe not worth the time and effort.
Tested manually though.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3023
Summary:
The import dialog now has an option to 'Update existing records',
which when checked will allow for selection of 1 or more fields
to match source and destination tables on.
If all fields match, then the matched record in the
destination table will be merged with the incoming record
from the source table. This means the incoming values will
replace the destination table values, unless the incoming
values are blank.
Additional merge strategies are implemented in the data
engine, but the import dialog only uses one of the
strategies currently. The others can be exposed in the UI
in the future, and tweak the behavior of how source
and destination values should be merged in different contexts,
such as when blank values exist.
Test Plan: Python and browser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3020
Summary: This adds a dropdown to the document settings model in staging/dev to set the python engine to Python2 or Python3. The setting is saved in `_grist_DocInfo.documentSettings.engine`.
Test Plan: tested manually for now - separate diff needed to add runsc to jenkins setup and make this testable
Reviewers: dsagal, alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3014
Summary:
Chart view used to rely on the same view field configuration as used in any other widget.
This diff allows to explicitely select X-AXIS, Y-AXIS and group by column with column picker.
As charts supports several y-axis, we still use a draggable list to arrange them.
Diff also fix doc to the `insertPositions` function.
Test Plan: Updated the relevant test.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3021
Summary: This tests site deletion with and without a plan.
Test Plan: adding tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3017
Summary:
This adds a `site:delete` target to `cli.sh` for deleting sites. Sites should be specified by numeric org id, and for confirmation their name also needs to be given.
All the docs in the site are deleted permanently, and the workspaces, and the site, and the stripe customer (if any).
Test Plan: manual
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3015
Summary:
This can happen thanks to some badly-behaved extensions (e.g. lingvanex), and
results in errors such as "Cannot redefine property: isInput".
Test Plan:
Tested manually that Grist fails to load in Chrome with Lingvanex
extension, and loads successfully with this change.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3013
Summary:
Moves CSV and XLSX export urls under /download/, and
removes the document title query parameter which is now
retrieved from the backend.
Test Plan: No new tests. Existing tests that verify endpoints still function.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3010
Summary:
Bar chart was a bit broken when there were redundant values on the X axis: the bars’s height maps to the sum of all the corresponding y values, when the data that shows up on hover is only the last one.
It seems that plotly does not support redundant values in the x axis and in all Plotly examples (implementation relies on plotly) x values only have unique values.
This diff, fixes by making sure x axis has unique values. If user actually wants to plot groups, they'll have to use a summary charts.
Test Plan: tested manually
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3011
Summary: We still show up to 5 on regular-width screens.
Test Plan: Browser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3008
Summary:
The endpoints for exporting CSV and Excel are now under
/api/docs/:docId/ and are forwarded to a doc worker for export.
The Share Menu has been updated to use the new endpoints.
Test Plan: No new tests. Existing tests that verify endpoints work correctly.
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3007
Summary:
The problem: For a data-cleaning column (one that depends on itself) `doBulkUpdateRecord` calls `prevent_recalc(should_prevent=False)``
which is supposed to cause it to get calculated eventually.
But before that it calls `_do_doc_action` -> `apply_doc_action` -> `_bring_lookups_up_to_date` which recalculates
a lookup column which eventually calls `_recompute_step(allow_evaluation=False)` on the data-cleaning column
which shouldn't really do anything significant but it both modifies the set `self.recompute_map[node]`
and then removes it from the map after it's empty.
The solution: when `allow_evaluation=False` and `self._prevent_recompute_map[node]` is nonempty,
ensure `self.recompute_map[node]` is not modified,
and check the map directly (instead of `dirty_rows` which can now be separate) to see if the set is empty before cleanup.
Test Plan: Added a lookup column to test_self_trigger, ensured that this caused the test to fail without the other two changes in engine.py.
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3006
Summary:
Allows selecting by a reflist in another table. This generalises cursor-linking with a ref column, but now it's filter linking.
Added another case to LinkingState where the source column is a reflist to the target table, filtering by the id column.
Updated convertQueryFromRefs and related functions to handle this since the id column has no column ref. In this case the string 'id' is used instead of a number.
LinkingState also checks if the source value is a reflist and uses that as the list of filter values instead of a single-element list of the cell value.
Indirect linking also works, where the source and target columns both are both references to the same table. This was the plan for a source reflist and target ref column.
I was surprised to see it also works perfectly when both columns are reflists, and it filters rows where there's an intersection!
Adding rows to the target section using the selected source record for default values is iffy. When filtering by row ID, there's no column for defaults, so the new row disappears.
For a source reflist and target ref, the first value of the reflist is the default, which is okayish. When both are reflists, the full source reflist is the default for the target column.
This seems like a bit much but just using the first value seems a bit arbitrary when there's room for all of them?
While doing all this I noticed an unrelated bug which I fixed as I was refactoring. Previously cursor linking based on a reference column did not update the cursor in the link target
when the value of the selected reference cell changed. Now cursor linking uses a floating row model like most other cases to observe the value correctly.
Test Plan: Extended SelectByRefList test and fixture, added previously failing test to RightPanelSelectBy.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3004
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: It is important for linking to be maintained, or user could be gravely misled about material in other sections.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3003
Summary:
The warning about workspace write access would still be shown if
a user picked a workspace they had write access to, and cleared the
Name field in the Save Copy dialog. This fixes the condition for
showing the warning to not show it in this case, and adds a placeholder
to the Name field when it is blank.
Test Plan: Browser test.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D3002
