DEV: Avoid hard-coding notification types integer in client side code.

DEV: Avoid hard-coding notification types integer in client side code.

Co-authored-by: Bianca Nenciu nenciu.bianca@gmail.com

diff --git a/app/assets/javascripts/discourse/widgets/concerns/notification-types.js.es6.erb b/app/assets/javascripts/discourse/widgets/concerns/notification-types.js.es6.erb
new file mode 100644
index 0000000..476ea70
--- /dev/null
+++ b/app/assets/javascripts/discourse/widgets/concerns/notification-types.js.es6.erb
@@ -0,0 +1,3 @@
+<% Notification.types.each do |key, value| %>
+export const <%= key.upcase %>_TYPE = <%= value %>;
+<% end %>
diff --git a/app/assets/javascripts/discourse/widgets/notification-item.js.es6 b/app/assets/javascripts/discourse/widgets/notification-item.js.es6
index a0d01f3..57a695d 100644
--- a/app/assets/javascripts/discourse/widgets/notification-item.js.es6
+++ b/app/assets/javascripts/discourse/widgets/notification-item.js.es6
@@ -13,10 +13,12 @@ import { setTransientHeader } from "discourse/lib/ajax";
 import { userPath } from "discourse/lib/url";
 import { iconNode } from "discourse-common/lib/icon-library";
 
-const LIKED_TYPE = 5;
-const INVITED_TYPE = 8;
-const GROUP_SUMMARY_TYPE = 16;
-export const LIKED_CONSOLIDATED_TYPE = 19;
+import {
+  LIKED_TYPE,
+  INVITEE_ACCEPTED_TYPE,
+  GROUP_MESSAGE_SUMMARY_TYPE,
+  LIKED_CONSOLIDATED_TYPE
+} from "discourse/widgets/concerns/notification-types";
 
 createWidget("notification-item", {
   tagName: "li",
@@ -58,7 +60,7 @@ createWidget("notification-item", {
       return postUrl(attrs.slug, topicId, attrs.post_number);
     }
 
-    if (attrs.notification_type === INVITED_TYPE) {
+    if (attrs.notification_type === INVITEE_ACCEPTED_TYPE) {
       return userPath(data.display_username);
     }
 
@@ -112,7 +114,7 @@ createWidget("notification-item", {
     const scope =
       notName === "custom" ? data.message : `notifications.${notName}`;
 
-    if (notificationType === GROUP_SUMMARY_TYPE) {
+    if (notificationType === GROUP_MESSAGE_SUMMARY_TYPE) {
       const count = data.inbox_count;
       const group_name = data.group_name;
       return I18n.t(scope, { count, group_name });
diff --git a/test/javascripts/fixtures/notification_fixtures.js.es6 b/test/javascripts/fixtures/notification_fixtures.js.es6
index 0ebf8bd..bcfbadf 100644
--- a/test/javascripts/fixtures/notification_fixtures.js.es6
+++ b/test/javascripts/fixtures/notification_fixtures.js.es6
@@ -1,12 +1,15 @@
 /*jshint maxlen:10000000 */
-import { LIKED_CONSOLIDATED_TYPE } from "discourse/widgets/notification-item";
+import {
+  LIKED_CONSOLIDATED_TYPE,
+  REPLIED_TYPE
+} from "discourse/widgets/concerns/notification-types";
 
 export default {
   "/notifications": {
     notifications: [
       {
         id: 123,
-        notification_type: 2,
+        notification_type: REPLIED_TYPE,
         read: false,
         post_number: 2,
         topic_id: 1234,
diff --git a/test/javascripts/fixtures/site-fixtures.js.es6 b/test/javascripts/fixtures/site-fixtures.js.es6
index 25d1dc3..15f136b 100644
--- a/test/javascripts/fixtures/site-fixtures.js.es6
+++ b/test/javascripts/fixtures/site-fixtures.js.es6
@@ -1,4 +1,4 @@
-import { LIKED_CONSOLIDATED_TYPE } from "discourse/widgets/notification-item";
+import { LIKED_CONSOLIDATED_TYPE } from "discourse/widgets/concerns/notification-types";
 
 export default {
   "site.json": {

GitHub sha: fcb74222

The notification types already exist on the Site model, so this data is sent to the client in two distinct forms (though, arguably it was before as well).

The fixtures make it impossible to use the Site data throughout, so could we remove #notification_types from SiteSerializer?

1 Like

Is Site.notification_types used anywhere on the client-side?

It is. It’s used from app/assets/javascripts/discourse/widgets/notification-item.js.es6 which is the same file that declared all the constants. There may be other uses.

1 Like

The fixtures make it impossible to use the Site data throughout, so could we remove #notification_types from SiteSerializer?

Can I get more context about what you’re trying to achieve and how the constants that are imported on demand affects anything?

I’m adding an API so that plugins can define formatters for notifications and so I was looking to get the notification type ids and wondering why I had a choice, Site data or generated constants.

@tgxworld, to be clear, I’m not asking you to do anything other than voice your opinion. I’m trying to see if people are broadly in favor of removing notifications_types from the SiteSerializer.

2 Likes

O icic. I’m ok with either as long as we don’t hard code the integers on the client side.

2 Likes

I prefer to keep site serializer pattern and dump the erb approach

1 Like

@SamSaffron Can you expand on why you think we should go with the site serializer pattern instead? Right now, we’re unnecessarily generating the notification_types on every page load whereas having the types in a JS file will allow us to generate it once and during precompilation.

DEV: Use `this.site.get("notification_types")` instead.

The erb pattern is less “flexible” in that if a plugin introduces these things and then is disabled on some multisites it is going to be super hard to erase the data from 1 site in the multisite.

1 Like