Email blocklisting #718

已合并
epsilon-phase 4 年前 将 9 次代码提交从 email-blocklisting 合并至 master
epsilon-phase 评论于 4 年前 (从 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] 评论于 4 年前 (从 github.com 迁移)

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 (从 github.com 迁移) 评审于 4 年前
igalic (从 github.com 迁移) 留下了一条评论

very very tired 👀

very very tired 👀
igalic (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评审于 4 年前
epsilon-phase (从 github.com 迁移) 评论于 4 年前

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

Fixed.

Oh, that's because rustfmt can't touch the templates :) Fixed.
elegaanz (从 github.com 迁移) 评审于 4 年前
elegaanz (从 github.com 迁移) 留下了一条评论

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!
elegaanz (从 github.com 迁移) 评论于 4 年前

Maybe make this column unique, to avoid duplicated blocks?

Maybe make this column unique, to avoid duplicated blocks?
elegaanz (从 github.com 迁移) 评论于 4 年前

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.
elegaanz (从 github.com 迁移) 评论于 4 年前
You could use https://doc.rust-lang.org/std/result/enum.Result.html#method.err
elegaanz (从 github.com 迁移) 评论于 4 年前

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).
elegaanz (从 github.com 迁移) 评论于 4 年前

It should probably redirect to /admin/emails instead.

It should probably redirect to `/admin/emails` instead.
elegaanz (从 github.com 迁移) 评论于 4 年前

It should probably redirect to /admin/emails.

It should probably redirect to `/admin/emails`.
elegaanz (从 github.com 迁移) 评论于 4 年前

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(
elegaanz (从 github.com 迁移) 评论于 4 年前

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)
elegaanz (从 github.com 迁移) 评论于 4 年前

This link is absent from the "Instances" page.

This link is absent from the "Instances" page.
elegaanz (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评论于 4 年前

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(&[
elegaanz (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评审于 4 年前
epsilon-phase (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评审于 4 年前
@ -177,0 +205,4 @@
}
#[post("/admin/emails/new", data = "<form>")]
pub fn add_email_blocklist(
epsilon-phase (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评审于 4 年前
epsilon-phase (从 github.com 迁移) 评论于 4 年前

That makes sense.

That makes sense.
epsilon-phase (从 github.com 迁移) 评审于 4 年前
epsilon-phase (从 github.com 迁移) 评论于 4 年前

Might as well follow the conventions :)

Might as well follow the conventions :)
epsilon-phase (从 github.com 迁移) 评审于 4 年前
epsilon-phase (从 github.com 迁移) 评论于 4 年前

That's much nicer :)

That's much nicer :)
elegaanz (从 github.com 迁移) 评审于 4 年前
@ -177,0 +205,4 @@
}
#[post("/admin/emails/new", data = "<form>")]
pub fn add_email_blocklist(
elegaanz (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评审于 4 年前
elegaanz (从 github.com 迁移) 留下了一条评论

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!)
elegaanz (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评论于 4 年前

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 (从 github.com 迁移) 评审于 4 年前
epsilon-phase (从 github.com 迁移) 评论于 4 年前

What an odd mystery

What an odd mystery
epsilon-phase (从 github.com 迁移) 评审于 4 年前
epsilon-phase (从 github.com 迁移) 评论于 4 年前

That makes sense

That makes sense
elegaanz (从 github.com 迁移)4 年前 批准此合并请求
elegaanz (从 github.com 迁移) 留下了一条评论

Perfect! Thank you!

Perfect! Thank you!

评审人

该合并请求已作为 f3c05dae62 被合并。
你也可以查看 命令行指令

第一步:

从你的仓库中签出一个新的分支并测试变更。
git checkout -b email-blocklisting master
git pull origin email-blocklisting

第二步:

合并变更并更新到 Forgejo 上
git checkout master
git merge --no-ff email-blocklisting
git push origin master
登录 并参与到对话中。
无审核者
未选择里程碑
未指派成员
1 名参与者
通知
到期时间
到期日期无效或超出范围。请使用 'yyyy-mm-dd' 格式。

未设置到期时间。

依赖工单

没有设置依赖项。

参考:Plume/Plume#718
正在加载...
这个人很懒,什么都没留下。