Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ticket/9837] Display unapproved posts to their authors #5222

Closed
wants to merge 2 commits into from
Closed

[ticket/9837] Display unapproved posts to their authors #5222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5917de
[ticket/9837] Display unapproved posts to their authors
CHItA May 11, 2018
35caaa0
[ticket/9837] Small fixes
CHItA May 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 14 additions & 2 deletions phpBB/phpbb/content_visibility.php
View file
Open in desktop
Expand Up @@ -144,7 +144,13 @@ public function get_count($mode, $data, $forum_id)
*/
public function is_visible($mode, $forum_id, $data)
{
$is_visible = $this->auth->acl_get('m_approve', $forum_id) || $data[$mode . '_visibility'] == ITEM_APPROVED;
$visibility = $data[$mode . '_visibility'];
$poster_key = ($mode === 'topic') ? 'topic_poster' : 'poster_id';
$is_visible = $this->auth->acl_get('m_approve', $forum_id) || $visibility == ITEM_APPROVED;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to optimize this use
$visibility == ITEM_APPROVED || $this->auth->acl_get('m_approve', $forum_id)

$is_visible = $is_visible || (
($visibility == ITEM_UNAPPROVED || $visibility == ITEM_REAPPROVE)
&& $this->user->data['user_id'] === $data[$poster_key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I'm not mistaken, this is gonna show unapproved anonymous posts to anonymous users, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is now yes. I added it to the TODO list to handle it. I'm really just trying to mock up what changes are needed first so I can have a better idea on what changes would the database tables need to make this a bit more optimized (as of now all users would have a separate query which doesn't seem optimal).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this->user->data['user_id'] === $data[$poster_key] first? If is not the user, it not performs the 2 checks.

);

/**
* Allow changing the result of calling is_visible
Expand Down Expand Up @@ -216,7 +222,13 @@ public function get_visibility_sql($mode, $forum_id, $table_alias = '')
}
else
{
$where_sql .= $table_alias . $mode . '_visibility = ' . ITEM_APPROVED;
$field_name = ($mode === 'topic') ? 'topic_poster' : 'poster_id';
$visibility_query = $table_alias . $mode . '_visibility = ';

$where_sql .= '(' . $visibility_query . ITEM_APPROVED . ')';
$where_sql .= ' OR (';
$where_sql .= '(' . $visibility_query . ITEM_UNAPPROVED . ' OR ' . $visibility_query . ITEM_REAPPROVE . ')';
Copy link

@gooof gooof Jun 12, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This need a switched off, its not good for large databases. There is no optimization possible.

			$visibility_query = $table_alias . $mode . '_visibility = ';

			$where_sql .= $visibility_query . ITEM_APPROVED;
			if ($this->config['show_items_unapproved'])
			{
				$field_name = ($mode === 'topic') ? 'topic_poster' : 'poster_id';
				$where_sql .= ' OR (';
				$where_sql .= $db->sql_in_set($visibility_query, array(ITEM_UNAPPROVED, ITEM_REAPPROVE));
				$where_sql .= ' AND ' . $table_alias . $field_name . ' = ' . (int) $this->user->data['user_id'] . ' 
AND ' . $table_alias . $field_name . ' != ' . ANONYMOUS . ')';
			}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A config switch will be in my next PR, need to decide whther to default on (my preference) or off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it help sql optimisation if the visibility of unapproved posts to posters had a time limit (only unapproved posts in the last X days)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it does. I believe it would be quite bad for users to have the post disappear. They could think that it had been deleted....

$where_sql .= ' AND ' . $table_alias . $field_name . ' = ' . ((int) $this->user->data['user_id']) . ')';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May you please change this to the query builder to allow extensions to more easily change this query for their own needs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query is already using query builder (in the calling function), here we are just building a WHERE clause for query builder.

Copy link
Contributor

@brunoais brunoais Sep 10, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@v12mike query builder: #4006
https://area51.phpbb.com/phpBB/viewtopic.php?t=47306

Why not use the query builder all the way, including the boolean processing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an existing event 'core.phpbb_content_visibility_get_visibility_sql_before' in the middle of building the sql, which I think would get broken if the sql builder was used more extensively here. Can you see how to do it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I failed to see it.
It's sad but...
I rest my case.

}

return '(' . $where_sql . ')';
Expand Down
6 changes: 6 additions & 0 deletions phpBB/styles/prosilver/template/viewtopic_body.html
View file
Open in desktop
Expand Up @@ -294,6 +294,7 @@ <h3 <!-- IF postrow.S_FIRST_ROW -->class="first"<!-- ENDIF -->><!-- IF postrow.P
<!-- EVENT viewtopic_body_postrow_post_details_after -->

<!-- IF postrow.S_POST_UNAPPROVED -->
<!-- IF postrow.S_CAN_APPROVE -->
<form method="post" class="mcp_approve" action="{postrow.U_APPROVE_ACTION}">
<p class="post-notice unapproved">
<span><i class="icon fa-question icon-red fa-fw" aria-hidden="true"></i></span>
Expand All @@ -304,6 +305,11 @@ <h3 <!-- IF postrow.S_FIRST_ROW -->class="first"<!-- ENDIF -->><!-- IF postrow.P
{S_FORM_TOKEN}
</p>
</form>
<!-- ELSE -->
<p class="post-notice information">
{L_POST_UNAPPROVED}
</p>
<!-- ENDIF -->
<!-- ELSEIF postrow.S_POST_DELETED -->
<form method="post" class="mcp_approve" action="{postrow.U_APPROVE_ACTION}">
<p class="post-notice deleted">
Expand Down
4 changes: 4 additions & 0 deletions phpBB/styles/prosilver/theme/colours.css
View file
Open in desktop
Expand Up @@ -1151,3 +1151,7 @@ input.disabled {
background-color: #d41142;
color: #ffffff;
}

.information {
background-color: #b8d3e0;
}
1 change: 1 addition & 0 deletions phpBB/viewtopic.php
View file
Open in desktop
Expand Up @@ -1993,6 +1993,7 @@
'S_HAS_ATTACHMENTS' => (!empty($attachments[$row['post_id']])) ? true : false,
'S_MULTIPLE_ATTACHMENTS' => !empty($attachments[$row['post_id']]) && count($attachments[$row['post_id']]) > 1,
'S_POST_UNAPPROVED' => ($row['post_visibility'] == ITEM_UNAPPROVED || $row['post_visibility'] == ITEM_REAPPROVE) ? true : false,
'S_CAN_APPROVE' => $auth->acl_get('m_approve', $forum_id),
'S_POST_DELETED' => ($row['post_visibility'] == ITEM_DELETED) ? true : false,
'L_POST_DELETED_MESSAGE' => $l_deleted_message,
'S_POST_REPORTED' => ($row['post_reported'] && $auth->acl_get('m_report', $forum_id)) ? true : false,
Expand Down