haystack / nb

12 stars 10 forks source link

Helen/add spotlight log notification #135

Closed lihelennn closed 3 years ago

lihelennn commented 3 years ago

Example of new values stored in database (please ignore the interaction_trigger_type column) image

lihelennn commented 3 years ago

Thanks Helen for the PR! Things look good, but I have few comments: 1- in the migration file, "edit-spolightlogs-enum", can you please handle rollback properly. Right now, if we rollback using this file, it's going to delete data. Instead of dropping everything, what we need to drop is the types you've introduced in this migration. The least we can do is not to put anything under "down" and not drop anything. 2- in "add-comment-type-column, what is the difference between action and comment type? They seem similar to me and they overlap with "reply-request".

Thank you!

Thanks Jumana! Yes, I will handle the rollback in the down section correctly. I will probably remove the type, but that makes a lot of sense. Thanks for catching that!

My idea of "comment_type" was really what triggered the sync notifications to be populated, so that we can understand better why someone clicked on a notification (for example, did they see a recent activity blinking thread and click on it, or did they get a notification that they were tagged?). I thought that these were not necessarily "actions," as the user would still need to "click" on a "tag" notification to view it, but lmk if you have other thoughts as well! Thank you again!

JumanaFM commented 3 years ago

Thanks, Helen! for commenttype, do you mean what triggered the click? If yes, don't you think naming that column "trigger" is more meaningful? Also, can you please provide me with more examples of the logs? I couldn't imagine a comment with a type "tag", so examples will help me understand your thinking process ^^

lihelennn commented 3 years ago

Thanks, Helen! for commenttype, do you mean what triggered the click? If yes, don't you think naming that column "trigger" is more meaningful? Also, can you please provide me with more examples of the logs? I couldn't imagine a comment with a type "tag", so examples will help me understand your thinking process ^^

@JumanaFM Yes! I can provide some examples of logs.

I wanted to use these logs to understand what types of notifications would attract students the most!

How does notification_type sound for that column? Or notification_trigger_type?

Thank you!

lihelennn commented 3 years ago

Thanks Helen for the PR! Things look good, but I have few comments: 1- in the migration file, "edit-spolightlogs-enum", can you please handle rollback properly. Right now, if we rollback using this file, it's going to delete data. Instead of dropping everything, what we need to drop is the types you've introduced in this migration. The least we can do is not to put anything under "down" and not drop anything. 2- in "add-comment-type-column, what is the difference between action and comment type? They seem similar to me and they overlap with "reply-request".

Thank you!

I also realized that in edit-spotlightlogs-enum.js, my rollback default is to drop the type "enum_spotlight_logs_type", which does not delete any data:

        down: async (queryInterface, Sequelize) => {
               await queryInterface.sequelize.query('DROP TYPE IF EXISTS "enum_spotlight_logs_type";');
         }

However, to ensure that this doesn't necessarily mess with the enum that you're using for your logs, I will just not do anything. As far as I know, it seems that there's no way to remove values from an enum, so I will just do nothing in the rollback (if that works for you). Thank you again!!

JumanaFM commented 3 years ago

Thanks, Helen! for commenttype, do you mean what triggered the click? If yes, don't you think naming that column "trigger" is more meaningful? Also, can you please provide me with more examples of the logs? I couldn't imagine a comment with a type "tag", so examples will help me understand your thinking process ^^

@JumanaFM Yes! I can provide some examples of logs.

  • "tag" is really just when someone receives a notification that they were tagged in something. If they then click on that notification, I want to record that was a "tag" notification.
  • If someone receives a notification that there was a thread posted near them recently and they then click on that notification, then I will use "recent activity".
  • If any instructor posts a comment, then I will use "instructor comment"! I hope that clarifies things. ?

I wanted to use these logs to understand what types of notifications would attract students the most!

How does notification_type sound for that column? Or notification_trigger_type?

Thank you!

I see. I tend to follow a standard for naming columns in which i pick one word headers. That's why I suggested "trigger". If you feel strongly about the name, then notification_type sounds more reasonable that the other one. :)

Thank you! Jumana

lihelennn commented 3 years ago

Thanks, Helen! for commenttype, do you mean what triggered the click? If yes, don't you think naming that column "trigger" is more meaningful? Also, can you please provide me with more examples of the logs? I couldn't imagine a comment with a type "tag", so examples will help me understand your thinking process ^^

@JumanaFM Yes! I can provide some examples of logs.

  • "tag" is really just when someone receives a notification that they were tagged in something. If they then click on that notification, I want to record that was a "tag" notification.
  • If someone receives a notification that there was a thread posted near them recently and they then click on that notification, then I will use "recent activity".
  • If any instructor posts a comment, then I will use "instructor comment"! I hope that clarifies things. ?

I wanted to use these logs to understand what types of notifications would attract students the most! How does notification_type sound for that column? Or notification_trigger_type? Thank you!

I see. I tend to follow a standard for naming columns in which i pick one word headers. That's why I suggested "trigger". If you feel strongly about the name, then notification_type sounds more reasonable that the other one. :)

Thank you! Jumana

Thanks, Jumana! I changed the name to interaction_trigger_type after writing my last message but was not aware of your naming convention. I will fix the PR and just name it trigger.

lihelennn commented 3 years ago

@JumanaFM I have made the changes but let me know if there are any problems/questions or other changes to make. Thanks!