Skip to content

improve transfer ownership error message - #2651

Merged
aerosol merged 6 commits into
plausible:masterfrom
ruslandoga:transfer-ownership-fix
Feb 13, 2023
Merged

improve transfer ownership error message#2651
aerosol merged 6 commits into
plausible:masterfrom
ruslandoga:transfer-ownership-fix

Conversation

@ruslandoga

@ruslandoga ruslandoga commented Feb 7, 2023

Copy link
Copy Markdown
Contributor

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

bundlemon Bot commented Feb 7, 2023

Copy link
Copy Markdown

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

Comment thread lib/plausible_web/controllers/site/membership_controller.ex Outdated
Comment thread lib/plausible_web/controllers/site/membership_controller.ex
Comment thread test/plausible_web/controllers/site/membership_controller_test.exs Outdated
@ruslandoga
ruslandoga requested a review from aerosol February 7, 2023 08:50
@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)

@ruslandoga ruslandoga Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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?

@ruslandoga
ruslandoga force-pushed the transfer-ownership-fix branch from 45fffd8 to 9dd6be2 Compare February 10, 2023 08:13

@aerosol aerosol left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

Sorry, my bad.

@ruslandoga
ruslandoga requested a review from aerosol February 12, 2023 17:21

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

@ruslandoga ruslandoga Feb 13, 2023

Copy link
Copy Markdown
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
Copy Markdown
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