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.

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

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

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@CHItA
Member

CHItA commented May 11, 2018

Checklist:

  • Correct branch: master for new features; 3.2.x for fixes
  • Tests pass
  • Code follows coding guidelines
  • Commit follows commit message format

TODOs

  • Review and finalize styling styling
  • Handle anonymous users (exclude them)
  • Optimize query caching
  • Fix displayed values (viewforum shows the topic with -1 replies)
  • Add tests

https://tracker.phpbb.com/browse/PHPBB3-9837

[ticket/9837] Display unapproved posts to their authors
Basic functionality mock up.

PHPBB3-9837

@CHItA CHItA added this to the 3.3.0-a1 milestone May 11, 2018

@CHItA

This comment has been minimized.

Member

CHItA commented May 11, 2018

@hanakin When you have some spare time could you look at the Notice styling?

Also @phpbb/development-team if anyone has any inputs on how this should work and if I'm missing something any pointers would be much appriciated.

@@ -1151,3 +1151,7 @@ input.disabled {
background-color: #d41142;
color: #ffffff;
}
p.information {

This comment has been minimized.

@hanakin

hanakin May 11, 2018

Member

loose the p

@@ -304,6 +305,11 @@ <h3 <!-- IF postrow.S_FIRST_ROW -->class="first"<!-- ENDIF -->><!-- IF postrow.P
{S_FORM_TOKEN}
</p>
</form>
<!-- ELSE -->
<p class="information post-notice">

This comment has been minimized.

@hanakin

hanakin May 11, 2018

Member

post-notice should come first so information can override it.

This comment has been minimized.

@lavigor

lavigor Jun 9, 2018

Contributor

@hanakin could you give an article or something about this change please?
Because this SO answer as well as other ones state clearly that the order of classes defined in HTML is not important for overriding at all.

$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 || (

This comment has been minimized.

@Nicofuma

Nicofuma May 12, 2018

Member

personnaly I would just hide deleted messages to their author.
Be careful to handle anonymous, I don't think we want to display anything to anonymous.
Also, please split the test in 2 variables for visibility: one for regular users, one for the author

[ticket/9837] Small fixes
PHPBB3-9837
$is_visible = $this->auth->acl_get('m_approve', $forum_id) || $visibility == ITEM_APPROVED;
$is_visible = $is_visible || (
($visibility == ITEM_UNAPPROVED || $visibility == ITEM_REAPPROVE)
&& $this->user->data['user_id'] === $data[$poster_key]

This comment has been minimized.

@Nicofuma

Nicofuma May 13, 2018

Member

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

This comment has been minimized.

@CHItA

CHItA May 13, 2018

Member

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).

This comment has been minimized.

@gooof

gooof Jun 9, 2018

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

$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;

This comment has been minimized.

@gooof

gooof Jun 9, 2018

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

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

This comment has been minimized.

@gooof

gooof Jun 12, 2018

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 . ')';
			}
$where_sql .= '(' . $visibility_query . ITEM_APPROVED . ')';
$where_sql .= ' OR (';
$where_sql .= '(' . $visibility_query . ITEM_UNAPPROVED . ' OR ' . $visibility_query . ITEM_REAPPROVE . ')';
$where_sql .= ' AND ' . $table_alias . $field_name . ' = ' . ((int) $this->user->data['user_id']) . ')';

This comment has been minimized.

@brunoais

brunoais Jun 12, 2018

Contributor

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

@lavigor lavigor referenced this pull request Jun 13, 2018

Closed

[ticket/15682] Revamp PM to chat based (GSoC 2018) #5235

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment