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.

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/13713] User Mentions (GSoC 2018) #5225

Closed
wants to merge 91 commits into from
Closed

Conversation

lavigor
Copy link
Contributor

@lavigor lavigor commented May 17, 2018

This Pull Request is the result of GSoC 2018 Project “User Mentions”.

The Idea

This project is intended to add the ability to mention specific users in forum topics so that they will receive a notification (just like in many modern messengers and websites).

Affected modules

This PR is backwards-compatible and works well with phpBB 3.3's BBCode parser (that was newly added in phpBB 3.2).

How it works

Users can type the @ symbol in textarea fields on the board to trigger a dropdown with usernames and group names. This list is generated based on several factors (called 'sources'): e.g. friends and topic repliers are shown higher in the list (internally they are given higher priorities).

Besides general user and group sources, built-in name sources include:

  • friends,
  • topic repliers (open poster receives even higher priority),
  • board team members,
  • groups that current user is a member of.

Extension developers can add their own sources to give certain users and groups higher priorities based on other conditions.

Special cases taken into consideration:

Case Solution
Users should be able to mention any users/groups regardless of the priorities Exact matches are always shown on the top the list
In most cases users should not mention themselves Name of the current user is removed from the list
Both usernames and group names should have equal chances of appearing in the list Priorities for usernames and group names are normalized based on the highest priority value

The name of mentioned user/group becomes a link to its profile highlighed in the colour of that user/group.

Configuration

Users are able to disable mention notifications in the same way as for other types of notifications (on Edit notification options page in User Control Panel).
Administrators can enable/disable the entire mentions feature and set the maximum number of names shown in the dropdown list as well as the maximum number of names fetched from the server on each request (on Post settings page in Administration Control Panel).

Background

This feature uses the At.js library for displaying mention dropdown.
It also involves names caching both on frontend and backend to improve performance and reduce server load.

Screenshot
default
Unparsed BBCode Parsed BBCode
default default

Current Status

User Mentions work. There'll be a PR to development documentation for how to add a new name source in your extension.


Checklist:

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

Tracker ticket (set the ticket ID to your ticket ID):

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

@lavigor
Copy link
Contributor Author

@lavigor lavigor commented May 17, 2018

!set WIP

@lavigor
Copy link
Contributor Author

@lavigor lavigor commented May 17, 2018

Seems that the command no longer works...

@rubencm
Copy link
Member

@rubencm rubencm commented May 17, 2018

!set WIP :construction:

@CHItA CHItA added this to the 3.3.0-a1 milestone May 17, 2018
@meis2m
Copy link

@meis2m meis2m commented May 19, 2018

well done ...

@@ -24,5 +24,10 @@ phpbb_help_routing:
resource: help.yml
prefix: /help

phpbb_mention_controller:
path: /mention
Copy link

@gooof gooof May 21, 2018

Choose a reason for hiding this comment

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

Possible to rename the path like mention_json or mention_api?

Copy link
Contributor Author

@lavigor lavigor May 21, 2018

Choose a reason for hiding this comment

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

I don't think there's a need in introducing a suffix with underscore.

Copy link
Member

@Nicofuma Nicofuma Sep 13, 2018

Choose a reason for hiding this comment

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

I think we should start using a convention on that. Maybe something like /api/user/mention on this specific case @marc1706

@@ -5,6 +5,8 @@
var text_name = <!-- IF $SIG_EDIT -->'signature'<!-- ELSE -->'message'<!-- ENDIF -->;
var load_draft = false;
var upload = false;
var mention_url = '{UA_MENTION_URL}';
Copy link
Member

@hanakin hanakin May 25, 2018

Choose a reason for hiding this comment

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

I would prefer that these were handled via data attributes on a hidden input or other form of html tag to avoid using inline js cause eventually this will all be factored out. Might as well prep for it

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

@hanakin so even very huge scripts such as the one in plupload.html will eventually be converted to data- attributes or something similar?
Requiring additional code to parse them back to the original JS objects?

I haven't made any performance checks or tests but using a script block seems to me as a rather convenient solution. Probably there should be one inline block at the bottom instead of many other, requiring some additional template features like INCLUDEJS for accumulating script data.

IMHO I'll better leave it as is right now so that it can be changed the same way together with other variables when that change rolls out.

Copy link
Member

@CHItA CHItA Jun 6, 2018

Choose a reason for hiding this comment

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

The aim would be to get rid of inline js all together.

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

@CHItA I understand that. That is why I'm asking about the reasons for that.
Because I also use inline script blocks in my extensions (e.g. quickreply_init.html) and I wonder how I'm supposed to convert them in future versions (I understand that I can leave it as is but I'd like to follow the guidelines).

Copy link
Member

@CHItA CHItA Jun 6, 2018

Choose a reason for hiding this comment

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

The new theme will probably use some build system for both JS and CSS by default, which will allow us to use some nice features (minification, auto-prefixing etc). This is obviously not usable on inlined JS or CSS.

Probably there will be some backend changes to support some features both for the new theme itself and extensions as well. However, these changes are not yet fully specified so we will probably publish our recommendations for extensions when we release those changes.

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

@CHItA can't wait for those changes to be done :)
Are there any plans for a preprocessor (LESS/SASS) instead of plain CSS?

Copy link
Member

@hanakin hanakin Jun 6, 2018

Choose a reason for hiding this comment

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

There is never a reason to use inline scripts...just port it to its own file and include it using includejs. Using data attr for any template bar stuff etc... the use of minificatiin in the new theme coupled with the adoption of html2 will mean we can globalize most everything anyway at little cost.
The new theme will be a completely different beast and will not be compatible with any extensions now nor should as it will be released under 4.0.0 meaning no compatibility.

I am also looking into vuejs as an additional aspect as working with twig alone is problematic for certain circumstances where vue components just make it effortless and we gain the added benifit of less js for most of the interactivity.

But none of that is for certain still toying around with the idea and doing some tests

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

@hanakin the only reason I use inline scripts is exactly for template variables that cannot be inserted to the plain JS.
Almost all other JS stuff goes with INCLUDEJS.

Looking forward for new changes though.

Copy link
Member

@hanakin hanakin Jun 7, 2018

Choose a reason for hiding this comment

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

yes but you can instead use data for that and move everything to your js file.

@rubencm
Copy link
Member

@rubencm rubencm commented May 30, 2018

You have a problem with phpBB folder in lowercase

@senky
Copy link
Contributor

@senky senky commented May 30, 2018

Isn't so much queries adding too much load to the server? Isn't caching the queries a middle way between server load and accuracy of the suggestions (new users/groups will pop up only after cache is past ttl)? Was this discussed somewhere?

@lavigor
Copy link
Contributor Author

@lavigor lavigor commented May 30, 2018

@senky caching queries will also be done just a bit later. That part should be one of the most complicated to decide.

@@ -8,6 +8,22 @@
<!-- BEGIN listitem --><li><!-- END listitem -->
<!-- BEGIN listitem_close --></li><!-- END listitem_close -->

<!-- BEGIN mention -->
Copy link
Member

@hanakin hanakin May 30, 2018

Choose a reason for hiding this comment

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

is this temp code? the xsl stuff needs to be extrapolated out into the core. we should not be shipping any xsl in the bbcode.html file. Joshy needs to remove what he left there...

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

I wasn't aware of Joshy's plans for that and this place seemed to be the right.
Moved to the core.

There is also much legacy stuff like first and second passes I guess.
Sometimes it is not obvious to distinguish the legacy, and I suppose that new BBCodes should not be handled by that code.

@@ -989,6 +989,18 @@ fieldset.fields2 dl:hover dt label {
outline-color: rgba(19, 164, 236, 0.5);
}

.atwho-container .atwho-view ul li:hover,
Copy link
Member

@hanakin hanakin Jun 6, 2018

Choose a reason for hiding this comment

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

You can specify a class with this library so that you do not need to chain selectors

AND ' . $this->db->sql_in_set('u.user_type', [USER_NORMAL, USER_FOUNDER]) . '
AND ' . $this->db->sql_in_set('u.user_id', $user_ids),
]);
$res = $this->db->sql_query($query);
Copy link
Contributor

@senky senky Jun 6, 2018

Choose a reason for hiding this comment

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

A good habit is to use $result as a variable name.

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

I thought that was common for phpBB but now I realize that this habit probably came from extensions.
Changed to $result.

while ($row = $this->db->sql_fetchrow($res))
{
$this->cached_colors['users'][$row['user_id']] = $row['user_colour'];
}
Copy link
Contributor

@senky senky Jun 6, 2018

Choose a reason for hiding this comment

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

$this->db->sql_freeresult($res);

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

Done.

while ($row = $this->db->sql_fetchrow($res))
{
$this->cached_colors['groups'][$row['group_id']] = $row['group_colour'];
}
Copy link
Contributor

@senky senky Jun 6, 2018

Choose a reason for hiding this comment

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

$this->db->sql_freeresult($res);

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

Done.

@@ -1,5 +1,7 @@
<script type="text/javascript">
// <![CDATA[
var mention_url = '{UA_MENTION_URL}';
var mention_topic_id = '{S_TOPIC_ID}';
Copy link
Contributor

@senky senky Jun 6, 2018

Choose a reason for hiding this comment

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

S_ means switch, i.e. boolean value. Just use {TOPIC_ID} which is maybe already defined.

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

@senky S_TOPIC_ID is indeed already defined in functions.php.
That is why it is used here.

Copy link
Contributor Author

@lavigor lavigor Jun 6, 2018

Choose a reason for hiding this comment

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

According to guidelines, S_ stands for system, not for switch.

Copy link
Contributor

@senky senky Jun 6, 2018

Choose a reason for hiding this comment

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

Ah right, then it's OK.

@@ -190,6 +190,7 @@
define('BBCODE_ID_EMAIL', 10);
define('BBCODE_ID_FLASH', 11);
define('BBCODE_ID_ATTACH', 12);
define('BBCODE_ID_MENTION', 13);
Copy link
Member

@paul999 paul999 Jun 12, 2018

Choose a reason for hiding this comment

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

How is this going to work out with users having existing custum_bbcodes? Do they start with ID 13? If so, this might cause problems I assume with those. Maybe even a idea to define it completly as custom bbcode instead of hardcoded in core

Copy link
Contributor Author

@lavigor lavigor Jun 12, 2018

Choose a reason for hiding this comment

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

I suppose that existing custom BBCodes don't have the hardcoded constant values.
But probably this constant can be removed entirely because it does not seem that I need to use it anywhere with the new parser. It could be suitable for deprecated stuff that is not necessary with the newly added BBCode.

Copy link
Member

@paul999 paul999 Jun 12, 2018

Choose a reason for hiding this comment

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

The issue is that the first created custom_bbcode is ID 13:
image
No idea if that can cause issues, but I can see it might happen.

Copy link
Member

@hanakin hanakin left a comment

Our stupid file structure is starting to really annoy me with a need to split colors. Please combine all css in a new file called mentions.css except for the colors. And include the new file please.

$(txtarea).atwho({
at: "@",
displayTpl: function(data) {
var avatar = (data.avatar.src) ? "<img src='" + data.avatar.src + "'>" :
Copy link
Member

@hanakin hanakin Jun 15, 2018

Choose a reason for hiding this comment

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

Why not use es6 template structure here https://caniuse.com/#search=Template%20literal

@lavigor
Copy link
Contributor Author

@lavigor lavigor commented Jun 15, 2018

@hanakin what about admin.css? It needs the styles to be duplicated/included.
Maybe I can just leave it as is.

As for ES6, PhpStorm is complaining every time about var 😊
I just used the same style as in other places across the file.
Could I maybe refactor the script separately?

P.S. Needed to type this reply the second time, mobile Internet chewed my text 🙄

@lavigor
Copy link
Contributor Author

@lavigor lavigor commented Jun 15, 2018

@hanakin what is the target IE version for 3.3 BTW?
It was IE 8 previously, does your link mean that even IE 11 would not be supported from now on?

@hanakin
Copy link
Member

@hanakin hanakin commented Jun 16, 2018

We have not supported ie 8 since 3.0 we dropped all support for ie 10 in 3.2 and going forward after 3.3 we are adopting >1% , last 2 browsers, you can check the package.json for any changes

@hanakin
Copy link
Member

@hanakin hanakin commented Jun 16, 2018

No do not split admin.css my bad. We no longer really use our lint for js as it’s antiquated and broken. I am working on a fix to switch to xo which is based on eslint which is far better. So for now maybe externalize everything into a function call so we can easily refactor

Copy link
Member

@hanakin hanakin left a comment

when you get time can you provide a screenshot of the rendered html/css also due to the sheer amount of changes can you also please post the rendered html code, that would help a lot?

@@ -29,6 +29,11 @@
// ]]>
</script>

<!-- IF S_ALLOW_MENTIONS -->
<div id="mention_params" style="display: none;" data-mention-url="{U_MENTION_URL}" data-mention-names-limit="{S_MENTION_NAMES_LIMIT}" data-topic-id="{S_TOPIC_ID}"></div>
Copy link
Member

@hanakin hanakin Jun 16, 2018

Choose a reason for hiding this comment

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

just add the data to the form-buttons div

@@ -49,6 +49,12 @@

// ]]>
</script>

<!-- IF S_ALLOW_MENTIONS -->
<div id="mention_params" style="display: none;" data-mention-url="{U_MENTION_URL}" data-mention-names-limit="{S_MENTION_NAMES_LIMIT}" data-topic-id="{S_TOPIC_ID}"></div>
Copy link
Member

@hanakin hanakin Jun 16, 2018

Choose a reason for hiding this comment

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

same here just add the data to the format buttons div

@lavigor
Copy link
Contributor Author

@lavigor lavigor commented Jun 16, 2018

@hanakin
Screenshot:
default
Rendered HTML:

<div class="atwho-container">
	<div id="atwho-ground-message">
		<div class="atwho-view" id="at-view-64" style="display: none; top: 356.267px; left: 83.4503px;">
			<ul class="atwho-view-ul">
				<li class="mention-name cur">
					<span class="mention-avatar"><i class="fa fa-user"></i></span><span>test</span>
				</li>
				<li class="mention-name">
					<span class="mention-avatar"><i class="fa fa-group"></i></span><span>Administrators</span>
				</li>
				<li class="mention-name">
					<span class="mention-avatar"><i class="fa fa-group"></i></span><span>Global moderators</span>
				</li>
				<li class="mention-name">
					<span class="mention-avatar"><i class="fa fa-user"></i></span><span>LavIgor</span><span class="mention-rank">Site Admin</span>
				</li>
			</ul>
		</div>
	</div>
</div>

we are adopting >1% , last 2 browsers

According to caniuse.com, IE 11 complies with this requirement. As we've discussed with @CHItA IE is actually the same as Edge and is no longer supported, but I personally use it sometimes. 😊

No do not split admin.css

Should I split regular styles or leave them all as is now?

maybe externalize everything into a function call

I've done that already, the function is named handle_mentions.

@hanakin
Copy link
Member

@hanakin hanakin commented Jun 17, 2018

@lavigor for prosilver only separate out the colors, put the rest in a new css file called mention.css and import it before colours.css, please... hmm as I suspected the structure is actually pretty similar to the pm structure. Which is a port of https://codepen.io/hanakin/pen/vrgOGG?editors=1100. I will have some cleanup for you by Monday ;)

(function($) {
function handle_mentions(txtarea) {
var $mentionDataContainer = $('#format-buttons'),
Copy link
Member

@hanakin hanakin Jun 17, 2018

Choose a reason for hiding this comment

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

do not map to an id instead map to the first data element $([data-mention-url]) this way if the id should change it does not break the js

@@ -557,6 +557,11 @@ blockquote .codebox {
padding: 5px 3px;
}

/* Mention block */
.mention {
Copy link
Member

@hanakin hanakin Jun 17, 2018

Choose a reason for hiding this comment

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

where is this used?

Copy link
Contributor Author

@lavigor lavigor Jun 20, 2018

Choose a reason for hiding this comment

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

This is used in posts for handling the parsed mention BBCode.

Copy link
Member

@hanakin hanakin Jun 20, 2018

Choose a reason for hiding this comment

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

ok it should still be in the new file with everything else.

lavigor and others added 19 commits Jan 27, 2021
@marc1706
Copy link
Member

@marc1706 marc1706 commented Jan 27, 2021

Rebased on latest master

@lavigor
Copy link
Contributor Author

@lavigor lavigor commented Jan 27, 2021

@marc1706 thanks
Very sorry for such a delay

@marc1706
Copy link
Member

@marc1706 marc1706 commented Jan 27, 2021

No worries. Wanted to see how cleanly it applies and I guess based on tests there might be some incompatibilities we need to resolve. :)

@marc1706
Copy link
Member

@marc1706 marc1706 commented Mar 13, 2021

I've worked a bit on switching from the no longer developed atwho.js to zurb tribute:
master...marc1706:ticket/13713

@lavigor would you have some time to maybe check if the behavior is the same as previously expected?

@marc1706 marc1706 closed this May 3, 2021
@marc1706 marc1706 mentioned this pull request May 3, 2021
4 tasks
@marc1706
Copy link
Member

@marc1706 marc1706 commented May 3, 2021

I have now replaced this PR with #6206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment