Exploration Inconsistency when Corresponding Table is Deleted

Aritra8438 commented 1 year ago

Description

When we create an exploration, we need to choose a From table. If that table is deleted, the exploration based on that is not deleted automatically (in frontend) resulting in inconsistency (let's say them dangling explorations). if we then open the exploration, there shows an error Invalid pk "193" - object does not exist.

Adding a video file to better understand the situation (error view in 30th second):

https://user-images.githubusercontent.com/64671908/227758333-1361466c-9ced-4aa8-bd34-36f2b2d6e6c2.mp4

Expected behavior

There are some observations:

Even after the table is deleted, we can edit the dangling exploration and assign it to a new table. But we can't save it after the reassignment (throws error Not Found)
There needs to be at least one existing table for that, otherwise we need to delete the exploration.
Once reloaded, all the dangling explorations are removed from the explorations tab (that means, they were already deleted from the database).

Based on those, we may have two solutions,

Delete all those dangling explorations from the explorations tab once the corresponding tables are deleted.
Allow assigning dangling explorations to another table and successfully save them.

In both cases, the minimum we need is not to show an error Invalid pk "193" - object does not exist. that might not be interpreted by a user but rather to show them the root cause and alternative solutions.

To Reproduce

create a table, say table_1
create an exploration from table_1, say exploration_1
delete table_1
now open exploration_1, it will show an error.
reload the page, those dangling explorations won't show up again.

Environment

OS: Ubuntu 22.04.1 LTS
Browser: Google Chrome
Browser Version: 111.0.5563.111 (Official Build) (64-bit)

Aritra8438 commented 1 year ago

I want to work on this issue as well (if labels are appropriate). Also, please suggest a solution that you think will be better. The backend deletes the explorations once the corresponding table is deleted, but keeping those dangling explorations and allowing reassignment would lessen the user modification needed.

rajatvijay commented 1 year ago

@Aritra8438 I prefer the first option.

Delete all those dangling explorations from the explorations tab once the corresponding tables are deleted.

But note this only requires to re-fetch the queries for the schema once a table is deleted.

In both cases, the minimum we need is not to show an error Invalid pk "193" - object does not exist. that might not be interpreted by a user but rather to show them the root cause and alternative solutions.

I don't think we need to show any error if we re-fetch the queries.

In addition to this, I also suggest showing some kind of warning to the user inside the delete table confirmation modal by saying something like:

Deleting this table will also delete all of the explorations built on top of it. Are you sure you want to proceed?

cc: @pavish adding this to v0.1.3 since this seems important from the perspective of UX.

Aritra8438 commented 1 year ago

Sure, @rajatvijay, I will work on that. Refetching the explorations would solve this problem. Thank you.

Aritra8438 commented 1 year ago

adding this to v0.1.3 since this seems important from the perspective of UX.

Hi @rajatvijay, are backlogs prioritized over v0.1.3?

Also, to fix this bug I need to modify the files which are already modified in my previous PR. So, fixing this bug and making a pull request may generate merge conflicts. Should I proceed, or wait for the merge approval of the existing PR?

kgodey commented 1 year ago

@rajatvijay Please talk to @ghislaineguerin about the expected behavior here. I think the idea behind leaving the dangling explorations was to allow the user to recover from errors without needing to start from scratch.

pavish commented 1 year ago

@kgodey I think this might be a valid bug, not an intentional UX.

I remember the behaviour was that if a table is deleted, we would delete the associated exploration as well. However, when a column is deleted, we would show errors to help fix the exploration. Perhaps this is related to metadata changes and we somehow did not catch this.

Even if we did want to have dangling explorations, the error shown here is 'Invalid pk' instead of something like 'Table not found'.

kgodey commented 1 year ago

@pavish I agree this is a bug, I just wanted to double check what the intended UX should be (maybe there's something better than deleting the danging explorations?). I don't have time to follow up further on this, so I'll leave it to your and @ghislaineguerin's judgement.

ghislaineguerin commented 1 year ago

I remember when we discussed this with @mathemancer, the idea was to leave the exploration, even if the table had been deleted and allow users to point to another table, to replace it and not lose their exploration. Imagine if they had deleted the table by mistake, and then recover it. In this scenario, removing the dangling explorations could be detrimental to the user experience.

pavish commented 1 year ago

@ghislaineguerin It's not possible to recover an exploration where the base table is deleted, pointing to a different table (or if they recover the same table by importing again) will not be possible since the columns used are entirely different.

If we have to do that, that requires an entire new user flow to map columns to new table which would be cumbersome with links involved.

Aritra8438 commented 1 year ago

@pavish, so we are going with deleting those dangling expolorations? Should I start working on this issue?

pavish commented 1 year ago

@Aritra8438 I'm marking this issue as a draft until we have confirmation from @ghislaineguerin. It might take a while for us to discuss and come to a resolution.

Once this issue is ready to be worked in, we'll update the status as ready.

mathemancer commented 1 year ago

@pavish If we automatically delete explorations whenever underlying dependencies are deleted, it's not possible for users to recover through any means. If we don't automatically delete such explorations, then (even if they have to manually open up another tab and create another exploration based on the now-broken one) they can try to help themselves recover.

That said, I defer to @ghislaineguerin 's judgement either way.

ghislaineguerin commented 1 year ago

I maintain the idea that removing explorations without notifying users results in a poor experience, as it doesn't communicate the system status accurately. Users should know their explorations are broken or invalid rather than missing. This is important because tables may be deleted outside of Mathesar.

By leaving the explorations in place, users can identify those that are invalid and delete them manually, as there are no alternative recovery methods.

The ideal would be that they can recover, but since that isn't happening now, then we can at least try to keep the information on how that exploration was built if possible.

pavish commented 1 year ago

@mathemancer @ghislaineguerin

I understand not deleting an exploration when other dependencies are deleted. The issue here is when a base table is deleted. All the required endpoints do not work when the base table is deleted, mainly the get endpoint for the exploration, the run shows a different error, and the joinable_tables endpoint doesn't work (which is fine).

This is in contrast to deleting a joined table which will show an error with the missing columns and ability to recover it.

If we don't automatically delete such explorations, then (even if they have to manually open up another tab and create another exploration based on the now-broken one) they can try to help themselves recover. we can at least try to keep the information on how that exploration was built if possible.

So, users are already unable to recover from this case in any means, or understand why it fails.

Having said all of this, if the backend can support recovery or atleast an ability to return the proper error, the frontend can support this. My concern was more on the viability of supporting this on the backend rather than the UX.

pavish commented 1 year ago

I'm restricting this to maintainers, since whatever we decide here would require design work, backend work, and frontend work.

pavish commented 1 year ago

I'd like one last confirmation on the backend viability from @mathemancer before unmarking this from draft. When the base table is deleted, can the APIs respond in a similar way as to when a joined table is deleted?

@ghislaineguerin The UX updates we'll need for this particular state may have complications and would need review/confirmation from the backend team:

A proper error message
A recovery option if possible
The base table information is no longer present, so what happens on the base table selector?
If the base table selector is empty, what happens on the column selection & transformation panes?

For comparison, here's the difference when base table is deleted vs joined table is deleted:

Base table is deleted	Joined table is deleted

GET endpoint for query fails	GET endpoint returns the query
Run endpoint fails with random errors	Run endpoint return proper error
Base table select menu on the UI cannot show the selected table, because the information no longer present anywhere	The base table information is shown properly on the UI
Query cannot be recovered	Query can be recovered in some manner

mathesar-foundation / mathesar