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

Notifications #2591

Merged
merged 36 commits into from Aug 6, 2018

Conversation

Projects
None yet
4 participants
@scopeInfinity
Copy link
Member

scopeInfinity commented Jul 28, 2018

Notifications System #2489

  • Notification Page
  • Numbers are written on the button(red background) showing unseen notification
  • Student Notification on Thread Merge(#2492)
  • Notification to all on creation of announcement/upgrade of thread to an announcement
  • Notification to the author of the post, on replying.

@scopeInfinity scopeInfinity changed the title [WIP] Notifications Notifications Jul 28, 2018

@scopeInfinity scopeInfinity requested a review from andrewaikens87 Jul 28, 2018

scopeInfinity and others added some commits Jul 28, 2018

--
-- Name: notifications; Type: TABLE; Schema: public; Owner: -
--
CREATE TABLE notifications (

This comment was marked as resolved.

@KevinMackenzie

KevinMackenzie Jul 30, 2018

Contributor

We might want to add a Notification class in the Model to represent this table

type notifications_type NOT NULL,
metadata TEXT NOT NULL,
content TEXT NOT NULL,
from_user_id VARCHAR(255) NOT NULL,

This comment has been minimized.

@KevinMackenzie

KevinMackenzie Jul 30, 2018

Contributor

we may want this to be null so that we can have notifications that don't originate from a user

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented Jul 30, 2018

Thanks, @KevinMackenzie
Tried an attempt with Notification class as Model.

protected $notify_content;
/** @property @var string Notification information about redirection link */
protected $notify_metadata;
/** @property @var string Should $notify_source be ignored from $notify_target */

This comment has been minimized.

@KevinMackenzie

KevinMackenzie Jul 30, 2018

Contributor

should this be a bool?

private function handleForum($details) {
switch ($details['type']) {
case 'new_announcement':
$this->setNotifyMetadata(json_encode(array('component' => 'forum', 'page' => 'view_thread', 'thread_id' => $details['thread_id'])));

This comment has been minimized.

@KevinMackenzie

KevinMackenzie Jul 30, 2018

Contributor

Why do we need component / page in the metadata?

This comment has been minimized.

@scopeInfinity

scopeInfinity Aug 1, 2018

Author Member

The initial idea was to store url info in the metadata.
So that using $this->core->buildUrl we can resolve metadata into url, easily.

*
* @param array $details
*/
private function handleForum($details) {

This comment was marked as resolved.

@KevinMackenzie

KevinMackenzie Jul 30, 2018

Contributor

Instead of a catch-all handleForum function, split each switch case into its own function that takes explicitly named parameters.

$this->pushNotification($source_user_id, $type, $metadata, $content, $target_users_query, $additional_param, $ignore_self);
}
public function getUserNotifications($user_id, $show_all){

This comment has been minimized.

@KevinMackenzie

KevinMackenzie Jul 30, 2018

Contributor

This function should return an array of Notification objects

* @param string $additional_param Additional parameters to be appended
* @param bool $ignore_self Should ignore $source_user_id from target users
*/
public function pushNotification($source_user_id, $type, $metadata, $content, $target_users_query, $additional_param, $ignore_self){

This comment has been minimized.

@KevinMackenzie

KevinMackenzie Jul 30, 2018

Contributor

We should have one pushNotification function in the DatabaseQueries class that takes a Notification model instance and switches which operation occurs based on the Notifications state.


{% if notifications is empty %}
<tr>
<td colspan="8">No notification found</td>

This comment has been minimized.

@andrewaikens87

andrewaikens87 Aug 2, 2018

Member

It might be nice to have "No new notifications." be displayed for if notifications is empty when looking at the unread notifications and either "No notification found." or "No notifications." for the view all option.

private function notificationsHandler() {
$user_id = $this->core->getUser()->getId();
if(!empty($_GET['action'])){
if($_GET['action'] == 'open_notification' && is_numeric($_GET['nid']) && $_GET['nid'] >= 1) {

This comment has been minimized.

@andrewaikens87

andrewaikens87 Aug 2, 2018

Member

This will throw an undefined index if action is defined but nid is not

}
private function actAsForumReplyNotification($thread_id, $post_content, $target) {
// TODO: Redirect to post itself

This comment has been minimized.

@andrewaikens87

andrewaikens87 Aug 2, 2018

Member

In the URL you can simply put #<post_id> at the end, try using the search feature and inspecting element on a resulting post for an example.

<h2>Notifications</h2>
<table class="table table-bordered persist-area">
<tr class="info persist-header">
<td colspan="8" style="text-align: center">Course: {{ course }}</td>

This comment has been minimized.

@andrewaikens87

andrewaikens87 Aug 2, 2018

Member

It might be better to have "New notifications" and "All notifications" for the table since when a user is currently viewing the notifications page it is within a course.

@KevinMackenzie

This comment has been minimized.

Copy link
Contributor

KevinMackenzie commented Aug 2, 2018

Looks like you've addressed all the changes I suggested. I tested it out a bit and it looks like its working, but @andrewaikens87 is going to take another look at it.

@KevinMackenzie
Copy link
Contributor

KevinMackenzie left a comment

It all seems to work as desired. There are just a few organizational things that should be straight-forward to fix.

$results[] = new Notification($this->core, array(
'view_only' => true,
'id' => $row['id'],
'component' => $row['type'],

This comment has been minimized.

@KevinMackenzie

KevinMackenzie Aug 2, 2018

Contributor

Its confusing that you have a type field in the database that gets converted to a component field in the model, but that model also takes a type parameter that means something different. If you make type consistent with type in the database and maybe change type as it exists in php now to action it would be a lot more clear.

@@ -694,4 +694,13 @@ public function deleteGradeableForm() {
return $this->core->getOutput()->renderTwigTemplate("navigation/DeleteGradeableForm.twig");
}
public function listNotifications($notifications, $show_all) {

This comment has been minimized.

@KevinMackenzie

KevinMackenzie Aug 2, 2018

Contributor

We could probably put this in a NotificationsView class or just call this code in-line where this function gets invoked (NavigationController). It doesn't make much sense to have it be in the Navigation View

@andrewaikens87
Copy link
Member

andrewaikens87 left a comment

Good except for the failing travis test, I can help tomorrow if it is not fixed by then

@bmcutler bmcutler merged commit 32bdf0e into master Aug 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bmcutler bmcutler deleted the notifications branch Aug 6, 2018

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