WIP: FEATURE: Posts awaiting moderation (PR #14501)

:construction: :construction: :construction:

This PR is under heavy WIP but I’d like to have some feedback on what’s been done already and explain some of the things I made. I’ll update the PR description accordingly when it’s not a draft anymore.

So this PR aims to allow people to see their pending posts as described here: Can I view my own topic that is awaiting moderation? - feature - Discourse Meta. The approach here is to display a new section (named Pending) on the user activity page. As for the drafts, a counter is displayed to know at a glance how many pending posts a user has. Since a post awaiting moderation isn’t a real post until it’s approved, it won’t show in the All section. (like it was for the drafts previously)
For now the Pending section is always there but it would probably be nice to only show it when it makes sense (with a site setting for example).

I’ve just started to add specs for the ruby side of the code and there are none for the JS side for now. I’ve also made some small refactors, I’ll explain more things by commenting directly on the PR.

GitHub

As I was taking example on what was made for drafts, I noticed we actually didn’t need this code because currentUser.draft_count is already a computed property updated through MessageBus so it will change automatically even in a template.

We also don’t need to check the value of count as it’s already baked in the I18n lib. (exactly like in Ruby/Rails).

This doesn’t exist anymore on the server side.

This route doesn’t seem to be used anymore so I recycled it. :recycle:

This template probably needs to be reworked and to be split in partials?

Annotations weren’t up to date.

Here I refactored our custom enum by using the one that is shipped with ActiveRecord. I think we should probably migrate all our enums but I guess this should be discussed :grin:

Small refactor using delegates.

I’m not sure of the impact of this change on all the other locales. I hope not to break them all :sweat_smile:

Callbacks make it a bit hard to test in isolation things as they create a lot of side effects. Maybe this is an area that could be improved by using a pub/sub approach for example (I’m thinking about Wisper).

This pull request introduces 1 alert when merging 16de2f5d815178042052d93b8e520695023679ec into 9f626f27356b0f623c9b4b1b55c077eea286e2ad - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Not sure what the text should be when there is no more pending posts to display.

This pull request introduces 1 alert when merging 95fcd2a58e19bd7921f2f5a8e6ed6ba985e2ece7 into 12856ab8c2f0d843f533547e9c1719d3cde1c00b - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

I’ve just seen we have a linter that says we should not use other keys than one and other for English locales but it’s a bit weird, isn’t it? :thinking: The I18n lib already does what needs to be done for pluralization, so why we don’t want to leverage that?

  _refreshModel() {

I don’t remember but if format-data is just outputing text, please wrap it in a span

So putting another span in the existing span? Like this?

<span class="time">{{d-icon "far-clock" title="Pending"}}<span>{{format-date pending_post.created_at}}</span></span>

yes, it’s important to have a direct parent html element

Indeed not sure…

@gschlager what do you suggest in this case please?

The plural rules for English allow only one and other. Yes, the I18n implementations in Rails and JS support zero as well. The problem is that translation tools like Crowdin can’t handle zero in English source strings. They are ignored and can’t be translated into any other languages. That’s why there is a linter rule that prevents the usage of zero.

So, the recommendation is to use a different translation key if you don’t want to use the other translation when count is 0.

1 Like