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

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

igalic (Migrated from github.com) reviewed 10 months ago

very very tired 👀

templates/instance/emailblocklist.rs.html

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

epsilon-phase (Migrated from github.com) reviewed 10 months ago

templates/instance/emailblocklist.rs.html

Plume_migration_agent commented 10 months ago

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

Fixed.

elegaanz (Migrated from github.com) reviewed 10 months ago

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

migrations/postgres/2020-01-05-232816_add_blocklist/up.sql

Plume_migration_agent commented 10 months ago

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.

plume-models/src/blocklisted_emails.rs

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).

src/routes/instance.rs

Plume_migration_agent commented 10 months ago

It should probably redirect to /admin/emails instead.

Plume_migration_agent commented 10 months ago

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?

src/routes/instance.rs

												
			@@ -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?

templates/instance/admin.rs.html

												
			@@ -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.

templates/instance/emailblocklist.rs.html

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.

Plume_migration_agent commented 10 months ago

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

templates/instance/emailblocklist.rs.html

												
			@@ -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.

epsilon-phase (Migrated from github.com) reviewed 10 months ago

templates/instance/emailblocklist.rs.html

Plume_migration_agent commented 10 months ago

It deletes only the ones selected.

But yeah, that’s a good idea

epsilon-phase (Migrated from github.com) reviewed 10 months ago

src/routes/instance.rs

												
			@@ -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.

epsilon-phase (Migrated from github.com) reviewed 10 months ago

src/routes/instance.rs

Plume_migration_agent commented 10 months ago

That makes sense.

epsilon-phase (Migrated from github.com) reviewed 10 months ago

migrations/postgres/2020-01-05-232816_add_blocklist/up.sql

Plume_migration_agent commented 10 months ago

Might as well follow the conventions :)

epsilon-phase (Migrated from github.com) reviewed 10 months ago

plume-models/src/blocklisted_emails.rs

Plume_migration_agent commented 10 months ago

That’s much nicer :)

elegaanz (Migrated from github.com) reviewed 10 months ago

src/routes/instance.rs

												
			@@ -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.

elegaanz (Migrated from github.com) reviewed 10 months ago

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

templates/instance/emailblocklist.rs.html

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).

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.

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).

epsilon-phase (Migrated from github.com) reviewed 10 months ago

templates/instance/emailblocklist.rs.html

Plume_migration_agent commented 10 months ago

What an odd mystery

epsilon-phase (Migrated from github.com) reviewed 10 months ago

templates/instance/emailblocklist.rs.html

Plume_migration_agent commented 10 months ago

That makes sense

elegaanz (Migrated from github.com) approved these changes 10 months ago

Perfect! Thank you!

kiwii referenced this issue from a commit 3 months ago

Email blocklisting (#718) * Interface complete for the email blacklisting * Everything seems to work * Neutralize language * fix clippy warnings * Add missing spaces * Added matching test * Correct primary key datatype for postgresql * Address review comments * Add placeholder when empty. Fix missing 'i'

Reviewers

Plume_migration_agent approved these changes 10 months ago

The pull request has been merged as f3c05dae62.

Plume_migration_agent

No Label

No Milestone

No Assignees

1 Participants

Notifications

Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

#718 Email blocklisting

Codecov Report

Reviewers