estuary / flow

🌊 Continuously synchronize the systems where your data lives, to the systems where you _want_ it to live, with Estuary Flow. 🌊
https://estuary.dev
Other
530 stars 45 forks source link

validation: more intuitive error message for expected pub #1498

Closed mdibaiee closed 1 week ago

mdibaiee commented 1 week ago

Description:

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

mdibaiee commented 1 week ago

another publication has modified this spec; to avoid conflicts, please refresh the resources and try again

I think "spec" is still a very technical word and not quite as obvious what it means for the "spec" to have been modified. I'm trying to think through the eyes of a user who has just landed on our dashboard for the first time and has created a capture and a materialization and went to update it for the first time and got this error. We do use the word in the UI though, we call the editor "Advanced Specification Editor" and in the details page of tasks and collections we have "SPEC" tab, so perhaps I'm being pessimistic and it is a familiar word for users.

I feel like "refresh the resources" is also rather vague

I agree, it irks me as well... given that users need to start over in the UI and they need to fetch the remote spec again before continuing in flowctl, I guess we could say "please try the operation again on a fresh copy of specifications" or something of the sort?

kiahna-tucker commented 1 week ago

if a user is on the edit page and they re-load the page, then it will preserve their previous draft so they would just get the same error again. My understanding is that in the UI they'd need to cancel their edit entirely and start over from the beginning.

Yes Phil, that is correct.

mdibaiee commented 1 week ago

expected publication ID {expect_id} was not matched (it's actually {actual_id}): this usually means your changes have already been made; to avoid conflicts, please refresh the resources and confirm.

@dyaffe this case of a change already having been made, to me is more of a bug in the UI that needs to be fixed to be honest. I've hit this issue a few times, where I hit Save, the publication is published but I'm not redirected / the page is not refreshed as usual, so I end up trying to save again only to hit this error. The error would be misleading in other scenarios though (when multiple tabs edit the resource, etc.)

dyaffe commented 1 week ago

@mdibaiee everyone says that and we attempted to fix this but 100% of the time that a user has gotten this error message so far, it has been a double click in the UI.

mdibaiee commented 1 week ago

How about this? I would personally prefer if the ending was a bit more clear (a fresh copy of the spec might not be as clear), so please suggest if you have a better idea πŸ€”

expected publication ID {expect_id} was not matched (it's actually {actual_id}): your changes have already been published or another publication has modified this spec; please try again with a fresh copy of the spec.

psFried commented 1 week ago

We do use the word in the UI though, we call the editor "Advanced Specification Editor" and in the details page of tasks and collections we have "SPEC" tab

Yeah, this has been brought up a number of times before, and I think your point about "spec" feeling a bit too vague and technical is valid. However: We've previously decided to stick with that term consistently, with the idea that users would then learn its meaning. We do use it elsewhere, at least somewhat consistently, so I wouldn't want to change it here without also changing it in all the other places. And I have seen users using the term "spec" fluently in our slack, so there's at least some evidence that they're willing and able to catch on. I also haven't heard a term that seems significantly better (just my 2 cents), so my suggestion is that we stick with "spec".

everyone says that and we attempted to fix this but 100% of the time that a user has gotten this error message so far, it has been a double click in the UI.

Couple of things to point out: This error can also be surfaced to flowctl users, and in that context "your changes have already been made" would be quite incrorrect.

Also, I expect that this error will become much more common for flowctl users because of the recent changes to the control-plane and flowctl. The control-plane now inlines inferred schemas, and so it regularly modifies collection specs. To prevent users for accidentally overwriting those inferred schema changes, flowctl now automatically sets an expectPubId whenever you run pull-specs. So if a collection has had an inferred schema updated since the last time you ran pull-specs, then you'd always get this error.

I'm not in love with the phrasing "with a fresh copy of the spec.", but don't have anything better.

Ditto. I think we should go with it.