Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve transfer ownership error message #2651

Merged
merged 6 commits into from
Feb 13, 2023

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Feb 7, 2023

Changes

Fixes #2640 by adding a more descriptive error message in case a transfer to an invited (but not joined) user is requested.

Screenshot 2023-02-13 at 11 40 58

Tests

  • Automated tests have been added

Changelog

  • Entry has been added to changelog

Documentation

  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode

@ruslandoga ruslandoga requested a review from a team February 7, 2023 08:18
@bundlemon
Copy link

bundlemon bot commented Feb 7, 2023

BundleMon

Unchanged files (7)
Status Path Size Limits
static/css/app.css
515.18KB -
static/js/dashboard.js
298.36KB -
static/js/app.js
12.13KB -
static/js/embed.host.js
5.58KB -
static/js/embed.content.js
5.06KB -
tracker/js/plausible.js
748B -
static/js/applyTheme.js
314B -

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@ruslandoga ruslandoga force-pushed the transfer-ownership-fix branch 2 times, most recently from 5fa66a6 to 45fffd8 Compare February 7, 2023 09:18
@ruslandoga ruslandoga mentioned this pull request Feb 7, 2023
4 tasks

message =
case errors do
%{invitation: [error | _]} -> String.capitalize(error)
Copy link
Contributor Author

@ruslandoga ruslandoga Feb 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:invitation is not a schema field, but a custom error key.

|> unique_constraint([:email, :site_id],
  name: :invitations_site_id_email_index,
  error_key: :invitation,
  message: "invitation already exists"
)

Should we still concat the other possible errors? I'm worried it might lead to confusing messages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the first one is fine. If anything confusing comes up we can address on case by case basis?

Copy link
Member

@aerosol aerosol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests fail. Please don't force push with ongoing review, it makes it more difficult to track changes. Thx.

@ruslandoga
Copy link
Contributor Author

Sorry, my bad.


message =
case errors do
%{invitation: ["already sent" | _]} -> "Invitation has already been sent"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid failing tests, changing too much in a single PR, and to still show a reasonable error message, I decided to reuse the approach used for duplicate invitations above:

{"already sent", _} ->
"This invitation has been already sent. To send again, remove it from pending invitations first."

role: "admin"
})

conn = get(recycle(conn), redirected_to(conn, 302))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL: recycle, nice

@aerosol aerosol merged commit e9ba60c into plausible:master Feb 13, 2023
@ruslandoga ruslandoga deleted the transfer-ownership-fix branch March 15, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfer ownership to invited user results in HTTP 500
2 participants