#718 Email blocklisting

Merged
epsilon-phase merged 9 commits from email-blocklisting into master 10 months ago
epsilon-phase commented 10 months ago (Migrated from github.com)

This should address #587 and any concerns for the terminology hopefully.

This should address #587 and any concerns for the terminology hopefully.
codecov[bot] commented 10 months ago (Migrated from github.com)
Owner

Codecov Report

Merging #718 into master will decrease coverage by 0.18%.
The diff coverage is 27.89%.

@@            Coverage Diff             @@
##           master     #718      +/-   ##
==========================================
- Coverage    39.3%   39.11%   -0.19%     
==========================================
  Files          72       73       +1     
  Lines        9534     9676     +142     
  Branches     2275     2313      +38     
==========================================
+ Hits         3747     3785      +38     
- Misses       4714     4807      +93     
- Partials     1073     1084      +11
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/718?src=pr&el=h1) Report > Merging [#718](https://codecov.io/gh/Plume-org/Plume/pull/718?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/458baf5f78daec1b823e0bbd17eaa96c23ae92f8?src=pr&el=desc) will **decrease** coverage by `0.18%`. > The diff coverage is `27.89%`. ```diff @@ Coverage Diff @@ ## master #718 +/- ## ========================================== - Coverage 39.3% 39.11% -0.19% ========================================== Files 72 73 +1 Lines 9534 9676 +142 Branches 2275 2313 +38 ========================================== + Hits 3747 3785 +38 - Misses 4714 4807 +93 - Partials 1073 1084 +11 ```
igalic (Migrated from github.com) reviewed 10 months ago
igalic (Migrated from github.com) left a comment

very very tired 👀

Plume_migration_agent commented 10 months ago

I’m a bit confused as to why there are no spaces after , s in function calls, starting here

I'm a bit confused as to why there are no spaces after `, `s in function calls, starting here
epsilon-phase (Migrated from github.com) reviewed 10 months ago
Plume_migration_agent commented 10 months ago

Oh, that’s because rustfmt can’t touch the templates :)

Fixed.

Oh, that's because rustfmt can't touch the templates :) Fixed.
elegaanz (Migrated from github.com) reviewed 10 months ago
elegaanz (Migrated from github.com) left a comment

Globally it works fine, great job! I left comments about “details”, but after that it should be good!

Plume_migration_agent commented 10 months ago

Maybe make this column unique, to avoid duplicated blocks?

Maybe make this column unique, to avoid duplicated blocks?
Plume_migration_agent commented 10 months ago

Little detail, so if you don’t feel like doing it, don’t change, but: keywords in SQL usually are in all caps.

Little detail, so if you don't feel like doing it, don't change, but: keywords in SQL usually are in all caps.
Plume_migration_agent commented 10 months ago
You could use https://doc.rust-lang.org/std/result/enum.Result.html#method.err
Plume_migration_agent commented 10 months ago

Maybe add a @ so that in only matches one domain, and not all domains with the same ending (I doubt this case will ever be encountered actually, be just to be sure).

Maybe add a `@` so that in only matches one domain, and not all domains with the same ending (I doubt this case will ever be encountered actually, be just to be sure).
Plume_migration_agent commented 10 months ago

It should probably redirect to /admin/emails instead.

It should probably redirect to `/admin/emails` instead.
Plume_migration_agent commented 10 months ago

It should probably redirect to /admin/emails.

It should probably redirect to `/admin/emails`.
Plume_migration_agent commented 10 months ago

Maybe call it /admin/emails/delete if it is what it does?

Maybe call it `/admin/emails/delete` if it is what it does?
@@ -177,0 +205,4 @@
}

#[post("/admin/emails/new", data = "<form>")]
pub fn add_email_blocklist(
Plume_migration_agent commented 10 months ago

I’m almost sure I did something similar for user management, couldn’t the code be shared between the two?

I'm almost sure I did something similar for user management, couldn't the code be shared between the two?
@@ -14,6 +14,7 @@
(&uri!(instance::admin).to_string(), i18n!(ctx.1, "Configuration"), true),
(&uri!(instance::admin_instances: page = _).to_string(), i18n!(ctx.1, "Instances"), false),
(&uri!(instance::admin_users: page = _).to_string(), i18n!(ctx.1, "Users"), false),
(&uri!(instance::admin_email_blocklist: page=_).to_string(), i18n!(ctx.1, "Email blocklist"), false)
Plume_migration_agent commented 10 months ago

This link is absent from the “Instances” page.

This link is absent from the "Instances" page.
Plume_migration_agent commented 10 months ago

This button should probably be red (class="destructive" IIRC), and explicitly say that it deletes all blocklisted emails if it is what it does.

This button should probably be red (`class="destructive"` IIRC), and explicitly say that it deletes all blocklisted emails if it is what it does.
Plume_migration_agent commented 10 months ago

This <input> should be before the label IMO, as it is usually done with checkboxes.

This `<input>` should be before the label IMO, as it is usually done with checkboxes.
@@ -0,0 +6,4 @@
@(ctx:BaseContext, emails: Vec<BlocklistedEmail>, page:i32, n_pages:i32)
@:base(ctx, i18n!(ctx.1, "Blocklisted Emails"), {}, {}, {
<h1>@i18n!(ctx.1,"Blocklisted Emails")</h1>
@tabs(&[
Plume_migration_agent commented 10 months ago

There should probably be a <h1> here, like on other administration pages.

There should probably be a `<h1>` here, like on other administration pages.
epsilon-phase (Migrated from github.com) reviewed 10 months ago
Plume_migration_agent commented 10 months ago

It deletes only the ones selected.

But yeah, that’s a good idea

It deletes only the ones selected. But yeah, that's a good idea
epsilon-phase (Migrated from github.com) reviewed 10 months ago
@@ -177,0 +205,4 @@
}

#[post("/admin/emails/new", data = "<form>")]
pub fn add_email_blocklist(
Plume_migration_agent commented 10 months ago

We couldn’t see how to do that cleanly, since that’s significantly more complicated.

We couldn't see how to do that cleanly, since that's significantly more complicated.
epsilon-phase (Migrated from github.com) reviewed 10 months ago
Plume_migration_agent commented 10 months ago

That makes sense.

That makes sense.
epsilon-phase (Migrated from github.com) reviewed 10 months ago
Plume_migration_agent commented 10 months ago

Might as well follow the conventions :)

Might as well follow the conventions :)
epsilon-phase (Migrated from github.com) reviewed 10 months ago
Plume_migration_agent commented 10 months ago

That’s much nicer :)

That's much nicer :)
elegaanz (Migrated from github.com) reviewed 10 months ago
@@ -177,0 +205,4 @@
}

#[post("/admin/emails/new", data = "<form>")]
pub fn add_email_blocklist(
Plume_migration_agent commented 10 months ago

OK, let’s do it this way then, we can always refactor later if needed.

OK, let's do it this way then, we can always refactor later if needed.
elegaanz (Migrated from github.com) reviewed 10 months ago
elegaanz (Migrated from github.com) left a comment

Sorry, actually there is more… 😕 (but it should be the last this time!)

Plume_migration_agent commented 10 months ago

Actually, this button should probably be hidden if there is currently no blocked addresses, and a placeholder message should be shown (<p class="center"> is made for that).

Actually, this button should probably be hidden if there is currently no blocked addresses, and a placeholder message should be shown (`<p class="center">` is made for that).
Plume_migration_agent commented 10 months ago

Moving it inside of the <label> tag would be better, otherwise they are not on the same line, and it is hard to understand they are linked.

Moving it inside of the `<label>` tag would be better, otherwise they are not on the same line, and it is hard to understand they are linked.
Plume_migration_agent commented 10 months ago

There is a i missing in i18n here, it doesn’t show correctly (it shouldn’t even compile, but for some reason, it does).

There is a `i` missing in `i18n` here, it doesn't show correctly (it shouldn't even compile, but for some reason, it does).
epsilon-phase (Migrated from github.com) reviewed 10 months ago
Plume_migration_agent commented 10 months ago

What an odd mystery

What an odd mystery
epsilon-phase (Migrated from github.com) reviewed 10 months ago
elegaanz (Migrated from github.com) approved these changes 10 months ago
elegaanz (Migrated from github.com) left a comment

Perfect! Thank you!

Reviewers

Plume_migration_agent approved these changes 10 months ago
The pull request has been merged as f3c05dae62.
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.