Plume/Plume
9
18
Fork 24
Code Issues 177 Pull requests 8 Releases 8 Wiki Activity

Email blocklisting #718

Merged
epsilon-phase merged 9 commits from email-blocklisting into master 2020-01-12 18:41:36 +00:00
Conversation 1 Commits 9 Files changed 17 +341 -6
epsilon-phase commented 2020-01-08 01:20:55 +00:00 (Migrated from github.com)
Copy link

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 2020-01-08 02:11:41 +00:00 (Migrated from github.com)
Copy link

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 2020-01-08 07:59:55 +00:00
igalic (Migrated from github.com) left a comment
Copy link

very very tired 👀

very very tired 👀
templates/instance/emailblocklist.rs.html
igalic (Migrated from github.com) commented 2020-01-08 07:52:01 +00:00
Copy link

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 2020-01-08 19:28:38 +00:00
templates/instance/emailblocklist.rs.html
epsilon-phase (Migrated from github.com) commented 2020-01-08 19:28:38 +00:00
Copy link

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 2020-01-11 15:37:51 +00:00
elegaanz (Migrated from github.com) left a comment
Copy link

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

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
elegaanz (Migrated from github.com) commented 2020-01-11 15:26:26 +00:00
Copy link

Maybe make this column unique, to avoid duplicated blocks?

Maybe make this column unique, to avoid duplicated blocks?
elegaanz (Migrated from github.com) commented 2020-01-11 15:27:19 +00:00
Copy link

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-models/src/blocklisted_emails.rs
elegaanz (Migrated from github.com) commented 2020-01-11 15:28:57 +00:00
Copy link

You could use https://doc.rust-lang.org/std/result/enum.Result.html#method.err

You could use https://doc.rust-lang.org/std/result/enum.Result.html#method.err
elegaanz (Migrated from github.com) commented 2020-01-11 15:30:00 +00:00
Copy link

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).
src/routes/instance.rs
elegaanz (Migrated from github.com) commented 2020-01-11 15:36:26 +00:00
Copy link

It should probably redirect to /admin/emails instead.

It should probably redirect to `/admin/emails` instead.
elegaanz (Migrated from github.com) commented 2020-01-11 15:37:00 +00:00
Copy link

It should probably redirect to /admin/emails.

It should probably redirect to `/admin/emails`.
elegaanz (Migrated from github.com) commented 2020-01-11 15:37:28 +00:00
Copy link

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

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(
elegaanz (Migrated from github.com) commented 2020-01-11 15:32:53 +00:00
Copy link

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?
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)
elegaanz (Migrated from github.com) commented 2020-01-11 15:34:05 +00:00
Copy link

This link is absent from the "Instances" page.

This link is absent from the "Instances" page.
templates/instance/emailblocklist.rs.html
elegaanz (Migrated from github.com) commented 2020-01-11 15:24:02 +00:00
Copy link

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.
elegaanz (Migrated from github.com) commented 2020-01-11 15:25:49 +00:00
Copy link

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.
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(&[
elegaanz (Migrated from github.com) commented 2020-01-11 15:21:53 +00:00
Copy link

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 2020-01-11 20:42:09 +00:00
templates/instance/emailblocklist.rs.html
epsilon-phase (Migrated from github.com) commented 2020-01-11 20:42:09 +00:00
Copy link

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 2020-01-11 20:46:26 +00:00
src/routes/instance.rs
@ -177,0 +205,4 @@
}
#[post("/admin/emails/new", data = "<form>")]
pub fn add_email_blocklist(
epsilon-phase (Migrated from github.com) commented 2020-01-11 20:46:26 +00:00
Copy link

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 2020-01-11 20:48:10 +00:00
src/routes/instance.rs
epsilon-phase (Migrated from github.com) commented 2020-01-11 20:48:10 +00:00
Copy link

That makes sense.

That makes sense.
epsilon-phase (Migrated from github.com) reviewed 2020-01-11 21:24:44 +00:00
migrations/postgres/2020-01-05-232816_add_blocklist/up.sql
epsilon-phase (Migrated from github.com) commented 2020-01-11 21:24:43 +00:00
Copy link

Might as well follow the conventions :)

Might as well follow the conventions :)
epsilon-phase (Migrated from github.com) reviewed 2020-01-11 21:35:55 +00:00
plume-models/src/blocklisted_emails.rs
epsilon-phase (Migrated from github.com) commented 2020-01-11 21:35:54 +00:00
Copy link

That's much nicer :)

That's much nicer :)
elegaanz (Migrated from github.com) reviewed 2020-01-12 12:25:20 +00:00
src/routes/instance.rs
@ -177,0 +205,4 @@
}
#[post("/admin/emails/new", data = "<form>")]
pub fn add_email_blocklist(
elegaanz (Migrated from github.com) commented 2020-01-12 12:25:20 +00:00
Copy link

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 2020-01-12 12:37:52 +00:00
elegaanz (Migrated from github.com) left a comment
Copy link

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

Sorry, actually there is more… :confused: (but it should be the last this time!)
templates/instance/emailblocklist.rs.html
elegaanz (Migrated from github.com) commented 2020-01-12 12:35:00 +00:00
Copy link

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).
elegaanz (Migrated from github.com) commented 2020-01-12 12:36:08 +00:00
Copy link

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.
elegaanz (Migrated from github.com) commented 2020-01-12 12:37:01 +00:00
Copy link

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 2020-01-12 18:12:43 +00:00
templates/instance/emailblocklist.rs.html
epsilon-phase (Migrated from github.com) commented 2020-01-12 18:12:43 +00:00
Copy link

What an odd mystery

What an odd mystery
epsilon-phase (Migrated from github.com) reviewed 2020-01-12 18:14:06 +00:00
templates/instance/emailblocklist.rs.html
epsilon-phase (Migrated from github.com) commented 2020-01-12 18:14:05 +00:00
Copy link

That makes sense

That makes sense
elegaanz (Migrated from github.com) approved these changes 2020-01-12 18:41:17 +00:00
elegaanz (Migrated from github.com) left a comment
Copy link

Perfect! Thank you!

Perfect! Thank you!
Sign in to join this conversation.
Reviewers
No reviewers
elegaanz
Labels
Clear labels   
A: API

Related to the REST API

  
A: Backend

Code running on the server

  
A: Federation

Stuff related to Federation

  
A: Front-End

Related to the front-end

  
A: I18N

Translations, and related code

  
A: Meta

More about project management or code than the project itself

  
A: Security

   
Build

The building, or installation process of Plume

  
C: Bug

Something isn't working

  
C: Discussion

We need to talk

  
C: Enhancement

New feature or request

  
C: Feature

This is a new feature

  
Compatibility

Compatibility with different browsers, readers and OS

  
Dependency

Related to an external package that Plume uses

  
Design

UI/UX related issues and PRs

  
Documentation

   
Good first issue

Good for newcomers

  
Help welcome

Extra attention is needed

  
Mobile

Issues affecting only mobile UX

  
Rendering

How elements're rendered out for the end user

  
S: Blocked

Something else needs to be fixed first

  
S: Duplicate

This issue or pull request already exists

  
S: Incomplete

This PR is not complete yet

  
S: Instance specific

Issues concern a limited number of instances

  
S: Invalid

This doesn't seem right

  
S: Needs Voting/Discussion

Need to be discussed by the community (on Loomio)

  
S: Ready for review

This PR is ready to be reviewed

  
Suggestion

Proposed ideas worth considering

  
S: Voted on Loomio

This is issue has been created after a vote on Loomio

  
S: Wontfix

This will not be worked on

No labels
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Plume/Plume#718
No description provided.
Delete branch "email-blocklisting"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?