Localize the name of the Trash #42514
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
johnswanson
force-pushed
the
localize-trash-name
branch
3 times, most recently
from
ed668c1
to
b324e1b
Compare
johnswanson
force-pushed
the
make-trash-usable-feature-branch
branch
from
9a4cb8e
to
0e92c09
Compare
There are at least two spots where we can't just rely on the after-select hook, and select the collection name directly from the database: the Search and Collection API. In these cases we need to call `collection/maybe-localize-trash-name`, which will localize the name if the passed Collection is the Trash collection.
johnswanson
force-pushed
the
localize-trash-name
branch
from
b324e1b
to
ff2f1ea
Compare
noahmoss
reviewed
May 13, 2024
Comment on lines
+125
to
+127
| ;; Urgh. `collection/is-trash?` will select the Trash collection (cached) based on its `type`. But as of this | ||
| ;; migration, this `type` does not exist yet. Neither does the Trash collection though, so let's just ... make | ||
| ;; that so. |
There was a problem hiding this comment.
I've run into issues like this before...may be a good topic for a backend guild discussion at some point. Feels like the older a migration is, the more likely there will be conflicts like this in the tests, so maybe we can deprecate migration tests after a certain number of versions...
There was a problem hiding this comment.
I was thinking the same thing - especially for non-custom migrations. It seems pretty unlikely that an old SQL migration will start behaving unexpectedly after many versions (Clojure-based migrations seem more important to keep testing).
noahmoss
approved these changes
May 13, 2024
johnswanson
merged commit 0b15fa0
into
make-trash-usable-feature-branch
108 of 110 checks passed
johnswanson
added a commit
that referenced
this pull request
May 14, 2024
There are at least two spots where we can't just rely on the after-select hook, and select the collection name directly from the database: the Search and Collection API. In these cases we need to call `collection/maybe-localize-trash-name`, which will localize the name if the passed Collection is the Trash collection.
johnswanson
added a commit
that referenced
this pull request
May 14, 2024
There are at least two spots where we can't just rely on the after-select hook, and select the collection name directly from the database: the Search and Collection API. In these cases we need to call `collection/maybe-localize-trash-name`, which will localize the name if the passed Collection is the Trash collection.
sloansparger
pushed a commit
that referenced
this pull request
May 16, 2024
There are at least two spots where we can't just rely on the after-select hook, and select the collection name directly from the database: the Search and Collection API. In these cases we need to call `collection/maybe-localize-trash-name`, which will localize the name if the passed Collection is the Trash collection.
sloansparger
added a commit
that referenced
this pull request
May 17, 2024
* Migration to add `trashed_from_*` (#41529) We want to record where things were trashed *from* for two purposes: - first, we want to be able to put things back if they're "untrashed". - second, we want to be able to enforce permissions *as if* something is still in its previous location. That is, if we trash a card or a dashboard from Collection A, the permissions of Collection A should continue to apply to the card or dashboard (e.g. in terms of who can view it). To achieve this, collections get a `trashed_from_location` (paralleling their `location`) and dashboards/cards get a `trashed_from_collection_id` (paralleling their `collection_id`). * Create the trash collection on startup (#41535) * Create the trash collection on startup The trash collection (and its descendants) can't have its permissions modified. Note that right now, it's possible to move the Trash collection. I'll fix this when I implement the API for moving things to the Trash. * s/TRASH_COLLECTION_ID/trash-collection-id/g * Add a comment to explain null comparison * Move archived items to the trash (#41543) This PR is probably too big. Hopefully documenting all the changes made here will make it easier to review and digest. So: Tables with a `collection_id` had `ON DELETE SET NULL` on the foreign key. Since we only deleted collections in testing and development, this didn't really affect anything. But now that we are actually deleting collections in production, it's important that nothing accidentally get moved to the root collection, which is what `SET NULL` actually does. When we get an API request to update the `archived` flag on a card, collection, or dashboard, we either move the item *to* the trash (if `archived` is `true`) or *from* the trash (if `archived` is `false`). We set the `trashed_from_collection_id` flag as appropriate, and use it when restoring an item if possible. Because moving something to the trash by itself would have permissions implications (since permissions are based on the parent collection) we need to change the way permissions are enforced for trashed items. First, we change the definition of `perms-objects-set-for-parent-collection` so that it returns the permission set for the collection the object was *trashed from*, if it is archived. Second, when listing objects inside the Trash itself, we need to check permissions to make sure the user can actually read the object - usually, of course, if you can read a collection you can read the contents, but this is not true for the trash so we need to check each one. In this case we're actually a little extra restrictive and only return objects the user can *write*. The reasoning here is that you only really want to browse the Trash to see things you could "act on" - unarchive or permanently delete. So there's no reason to show you things you only have read permissions on. Because previously the Collections API expected archived collections to live anywhere, not just in a single tree based in the Trash, I needed to make some changes to some API endpoints. This endpoint still takes an `exclude-archived` parameter, which defaults to `false`. When it's `false`, we return the entire tree including the Trash collection and archived children. When it's `true`, we exclude the Trash collection (and its subtree) from the results. Previously, this endpoint took an `archived` parameter, which defaulted to `false`. Thus it would only return non-archived results. This is a problem, because we want the frontend to be able to ask for the contents of the Trash or an archived subcollection without explicitly asking for archived results. We just want to treat these like normal collections. The change made to support this was to just default `archived` to the `archived` status of the collection itself. If you're asking for the items in an archived collection, you'll only get archived results. If you're asking for the items in a non-archived collection, you'll only get unarchived results. This is, for normal collections, the same thing as just returning all results. But see the section on namespaced collections for details on why we needed this slightly awkward default. No change - this endpoint still takes an `archived` parameter. When `archived=true`, we return the Trash collection, as it is in the root collection. Otherwise, we don't. * Make Trash Usable - UI (#41666) * Remove Archive Page + Add `/trash` routing (#42226) * removes archive page and related resources, adds new /trash route for trash collection, adds redirects to ensure consistent /trash routing instead of collection path * fixes unit + e2e tests, corrects links generated for trash collection to use /trash over /collect/:trashId route * updates comment * Serialize trash correctly (#42345) Also, create the Trash in a migration rather than on startup. Don't set a specific trash collection.id, instead just select based on the `type` when necessary. * Fix collection data for trashed items (#42284) * Fix collection IDs for trashed items When something is in the trash, we need to check permissions on the `trashed_from_collection_id` rather than the `collection_id`. We were doing this. However, we want the actual collection data on the search result to represent the actual collection it's in (the trash). I added the code to do this, a test to make sure it works as intended, and a comment to explain what we're doing here and why. * Refactor permission for trashed_from_collection_id Noah pointed out that the logic of "what collection do I check for permissions" was getting sprinkled into numerous places, and it felt a little scary. In fact, there was a bug in the previous implementation. If you selected a collection with `(t2/select [:model/Collection ...])`, selecting a subset of columns, and *didn't* include the `trashed_from_collection_id` in that set of columns, then called `mi/can-write?` on the result, you'd get erroneous results. Specifically, we'd return `nil` (representing the root collection). I think this is a reasonable fix, though I'm pretty new to using fancy multimethods with `derive` and such. But basically, I wanted a way to annotate a model to say "check these permissions using `:trashed_from_collection_id` if the item is archived." And in this case, we'll throw an exception if `:trashed_from_collection_id` is not present, just like we currently throw an exception if `:collection_id` is not present when checking permissions the normal way. * Move existing archived items to the trash (#42241) Move everything that's archived directly to the trash. It's not ideal because we're wiping out the existing collection structure, but the existing Archive page also has no collection structure. * lint fixes from rebase * Fix backend tests (#42506) `can_write` on collection items was wrong because we didn't have a `trashed_from_collection_id` - the refactor to ensure we didn't make that mistake worked! 🎉 * Add an /api/collections/trash endpoint (#42476) This fetches the Trash in exactly the same way as if we'd fetched it with `/api/collection/:id` with the Trash ID. I hadn't realized that the frontend was doing this with the previously hardcoded Trash ID. * Make Trash Usable - Dynamic Trash Collection Id (#42532) * wip * fixes hardcoded reference to trash id in sidebar * remove TRAHS_COLLECTION * fixes line and e2e tests * fix invert logic mistake and fixes lint * Make Trash Usable - Search Filter UI (#42402) * adds filtering for archived items on the search page * fix typing mistake * Make Trash Usable - Bug Bash 1 (#42541) * disables reordering columns in archived questions, disables modifying archived question name in question header, collection picker no longer defaults to archived item, keeps trashed collection from appearing collection picker search results, shops showing empty area in trashed dashboard info sidebar, disables uploading csvs to trashed collections * impls pr feedback * fix e2e failure * Localize the name of the Trash (#42514) There are at least two spots where we can't just rely on the after-select hook, and select the collection name directly from the database: the Search and Collection API. In these cases we need to call `collection/maybe-localize-trash-name`, which will localize the name if the passed Collection is the Trash collection. * Update migration IDs Migration IDs need to be in order. * Fix failing mariadb:latest test (#42608) Hooooly cow. Glad to finally track this down. The issue was that booleans come back from MariaDB as NON BOOLEANS, and clojure says 0 is truthy, and together that makes for FUN TIMES. We need to turn MariaDB's bits to booleans before we can work with them. * Make Trash Usable: Add `can_restore` to API endpoints (#42654) Rather than having two separate implementations of the `can_restore` hydration for cards and dashboards, I set up automagic hydration for the `trashed_from_collection_id`. Then the hydration method for `can_restore` can just first hydrate `trashed_from_collection` and then operate based on that. * fix can_restore for collections trashed from root * Fix `trashed_from_location` for trashed subcols We were setting `trashed_from_location` on subcollections to the `trashed_from_location` of the ancestor that was trashed - which is wrong. This would have caused bugs where collections would be restored to the root collection, regardless of where they were originally trashed from. --------- Co-authored-by: Sloan Sparger <sloansparger@gmail.com> * Fix Trash rollbacks (#42710) * Fix Trash rollbacks There were a few errors in my initial implementation. First, we can't simply assume that `trashed_from_location` and `trashed_from_collection_id` are set for anything in the archive. They should be set for things *directly* trashed, but not for things trashed as part of a collection. Therefore, we need to set `location` and `collection_id` to something like "if the item is in the trash, then the `trashed_from` - otherwise, don't modify it". Second, because `trashed_from_collection_id` is not a foreign key but `collection_id` is, we have a potential problem. If someone upgrades, Trashes a dashboard, then Trashes and permanently deletes the collection that dashboard _was_ in, then downgrades, how do we move the dashboard "back"? What do we set the dashboard's `collection_id` to? The solution here is that when we downgrade, we don't actually move dashboards, collections, or cards out of the Trash collection. Instead we just make Trash a normal collection and leave everything in it. * Make Trash Usable - Bug Bash 2 (#42787) * wip * fixes access to property on null value * pr clean up * more clean up * Fix up tests - permissions will check the archived from location. So recents need to select :report_card.trashed_from_collection_id and :dash.trashed_from_collection_id to satisfy mi/can-read? - some commented out code with `(def wtf (mt/user-http-request ...))`. Restore tests. - probably commented out because the recent views come back in an order but we don't care about the order. And it was asserting against an ordered collection. Just go from [{:id <id> :model <model>} ...] to a map of {<id> <model>} and then our comparison works fine and is not sensitive to order. * put try/finally around read-only-mode ensure the setting read-only-mode! to false happens in a finally block. Also, set read-only-mode to false in ```clojure (deftest read-only-login-test (try (cloud-migration/read-only-mode! true) (mt/client :post 200 "session" (mt/user->credentials :rasta)) (finally (cloud-migration/read-only-mode! false)))) ``` But worryingly, I think we have a lurking problem that I'm not sure why we haven't hit yet. We run tests in parallel and then put all of the non-parallel tests on the same main thread. And when one of these puts the app in read-only-mode, the parallel tests will fail. HOPEFULLY since they are parallel they won't be hitting the app-db necessarily, but there could be lots of silly things that break. --------- Co-authored-by: John Swanson <john.swanson@metabase.com> Co-authored-by: dan sutton <dan@dpsutton.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #42505
There are at least two spots where we can't just rely on the
after-select hook, and select the collection name directly from the
database: the Search and Collection API.
In these cases we need to call
collection/maybe-localize-trash-name,which will localize the name if the passed Collection is the Trash
collection.