Plume/Plume
9
18
Förgrening 24
Kod Ärenden 177 Pull-förfrågningar 8 Släpp 8 Wiki Aktiviteter

Email blocklisting #718

Sammanfogat
epsilon-phase sammanfogade 9 incheckningar från email-blocklisting in i master 2020-01-12 18:41:36 +00:00
Konversation 1 Incheckningar 9 Ändrade filer 17 +341 -6
epsilon-phase kommenterad 2020-01-08 01:20:55 +00:00 (Migrerad från github.com)
Kopiera länk

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

This should address #587 and any concerns for the terminology hopefully.
codecov[bot] kommenterad 2020-01-08 02:11:41 +00:00 (Migrerad från github.com)
Kopiera länk

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 (Migrerad från github.com) granskad av 2020-01-08 07:59:55 +00:00
igalic (Migrerad från github.com) lämnade en kommentar
Kopiera länk

very very tired 👀

very very tired 👀
templates/instance/emailblocklist.rs.html
igalic (Migrerad från github.com) kommenterad 2020-01-08 07:52:01 +00:00
Kopiera länk

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 (Migrerad från github.com) granskad av 2020-01-08 19:28:38 +00:00
templates/instance/emailblocklist.rs.html
epsilon-phase (Migrerad från github.com) kommenterad 2020-01-08 19:28:38 +00:00
Kopiera länk

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

Fixed.

Oh, that's because rustfmt can't touch the templates :) Fixed.
elegaanz (Migrerad från github.com) granskad av 2020-01-11 15:37:51 +00:00
elegaanz (Migrerad från github.com) lämnade en kommentar
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-11 15:26:26 +00:00
Kopiera länk

Maybe make this column unique, to avoid duplicated blocks?

Maybe make this column unique, to avoid duplicated blocks?
elegaanz (Migrerad från github.com) kommenterad 2020-01-11 15:27:19 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-11 15:28:57 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-11 15:30:00 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-11 15:36:26 +00:00
Kopiera länk

It should probably redirect to /admin/emails instead.

It should probably redirect to `/admin/emails` instead.
elegaanz (Migrerad från github.com) kommenterad 2020-01-11 15:37:00 +00:00
Kopiera länk

It should probably redirect to /admin/emails.

It should probably redirect to `/admin/emails`.
elegaanz (Migrerad från github.com) kommenterad 2020-01-11 15:37:28 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-11 15:32:53 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-11 15:34:05 +00:00
Kopiera länk

This link is absent from the "Instances" page.

This link is absent from the "Instances" page.
templates/instance/emailblocklist.rs.html
elegaanz (Migrerad från github.com) kommenterad 2020-01-11 15:24:02 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-11 15:25:49 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-11 15:21:53 +00:00
Kopiera länk

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 (Migrerad från github.com) granskad av 2020-01-11 20:42:09 +00:00
templates/instance/emailblocklist.rs.html
epsilon-phase (Migrerad från github.com) kommenterad 2020-01-11 20:42:09 +00:00
Kopiera länk

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 (Migrerad från github.com) granskad av 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 (Migrerad från github.com) kommenterad 2020-01-11 20:46:26 +00:00
Kopiera länk

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 (Migrerad från github.com) granskad av 2020-01-11 20:48:10 +00:00
src/routes/instance.rs
epsilon-phase (Migrerad från github.com) kommenterad 2020-01-11 20:48:10 +00:00
Kopiera länk

That makes sense.

That makes sense.
epsilon-phase (Migrerad från github.com) granskad av 2020-01-11 21:24:44 +00:00
migrations/postgres/2020-01-05-232816_add_blocklist/up.sql
epsilon-phase (Migrerad från github.com) kommenterad 2020-01-11 21:24:43 +00:00
Kopiera länk

Might as well follow the conventions :)

Might as well follow the conventions :)
epsilon-phase (Migrerad från github.com) granskad av 2020-01-11 21:35:55 +00:00
plume-models/src/blocklisted_emails.rs
epsilon-phase (Migrerad från github.com) kommenterad 2020-01-11 21:35:54 +00:00
Kopiera länk

That's much nicer :)

That's much nicer :)
elegaanz (Migrerad från github.com) granskad av 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 (Migrerad från github.com) kommenterad 2020-01-12 12:25:20 +00:00
Kopiera länk

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 (Migrerad från github.com) granskad av 2020-01-12 12:37:52 +00:00
elegaanz (Migrerad från github.com) lämnade en kommentar
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-12 12:35:00 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-12 12:36:08 +00:00
Kopiera länk

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 (Migrerad från github.com) kommenterad 2020-01-12 12:37:01 +00:00
Kopiera länk

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 (Migrerad från github.com) granskad av 2020-01-12 18:12:43 +00:00
templates/instance/emailblocklist.rs.html
epsilon-phase (Migrerad från github.com) kommenterad 2020-01-12 18:12:43 +00:00
Kopiera länk

What an odd mystery

What an odd mystery
epsilon-phase (Migrerad från github.com) granskad av 2020-01-12 18:14:06 +00:00
templates/instance/emailblocklist.rs.html
epsilon-phase (Migrerad från github.com) kommenterad 2020-01-12 18:14:05 +00:00
Kopiera länk

That makes sense

That makes sense
elegaanz (Migrerad från github.com) godkände dessa ändringar 2020-01-12 18:41:17 +00:00
elegaanz (Migrerad från github.com) lämnade en kommentar
Kopiera länk

Perfect! Thank you!

Perfect! Thank you!
Logga in för att delta i denna konversation.
Granskare
Inga granskare
elegaanz
Etiketter
Rensa etiketter   
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

Inga etiketter
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
Milsten
Rensa milstenar
Inga objekt
Ingen milstolpe
Projekt
Rensa projekt
Inga objekt
Inget projekt
Tilldelade
Rensa tilldelade
Inga tilldelade
1 participant
Notiser
Förfallodatum
Förfallodatumet är ogiltigt eller utanför gränserna. Använd formatet "åååå-mm-dd".

Inget förfallodatum satt.

Beroenden

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?