Integration of Chatkit #406

flxffy · 2019-07-02T15:23:33Z

Backend to supplement the usage of ChatKit. API can be found here.

What's new:

lib/cadet/chat/token.ex: generate tokens for connection to Chatkit's services
lib/cadet/chat/room.ex: logic for creating rooms using Chatkit's API and storing room id in the db
2.1 Updated router to enable frontend retrieval of a token - for Chatkit's TokenProvider in the frontend
mix/tasks/users/chat.ex: logic for creating users on Chatkit

Current Implementation:

Rooms are currently being created at three points - when answers are created, when students edit their answers and when cadets access answer in the grading view. In the event Chatkit crashes, the last two points serve as failsafes to ensure comments get to the students.
- Alternative: a script to create all chatrooms after Chatkit restores its services, if it crashes
Making use of comment column in the Answer table to store room ids

Tests for Chatkit will be included by the end of the week once I figure out how ExVCR works 👯

…of Chatkit's server failure. Untested.

…bmit

coveralls · 2019-07-02T15:49:29Z

Pull Request Test Coverage Report for Build 2513

27 of 39 (69.23%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.2%) to 95.829%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/cadet/chat/room.ex	19	20	95.0%
lib/cadet_web/router.ex	0	1	0.0%
lib/cadet_web/views/chat_view.ex	0	1	0.0%
lib/cadet/chat/token.ex	4	6	66.67%
lib/cadet_web/controllers/chat_controller.ex	0	7	0.0%

Totals
Change from base Build 2512:	-1.2%
Covered Lines:	850
Relevant Lines:	887

💛 - Coveralls

geshuming

Hi, first thing I noticed is that there isn't a lot of tests...

Can we get some tests written so we can at least understand how to approach and use your features.

flxffy · 2019-07-02T17:07:01Z

Hi, first thing I noticed is that there isn't a lot of tests...

Can we get some tests written so we can at least understand how to approach and use your features.

@geshuming There are no tests at the moment! They will be included once I figure out how ExVCR works. I'll ping the group once they are complete and ready for merging!

geshuming

Some preliminary comments.

I realised that you are reusing the comment field from answer. Can we deprecate the comment field instead, and add a new field for the chat room token.

Since the frontend no longer requires avengers to write grading comments, the frontend should send null or empty string for the comment, and we can further guard the field here by having it default to empty string, or be nullable in the database. Regardless, we can keep the comment field but mark it as deprecated.

README.md

lib/cadet/assessments/assessments.ex

geshuming

Ok, looks better! We need a couple more optimizations, otherwise it will be good to go.

lib/cadet/assessments/assessments.ex

lib/cadet/chat/room.ex

lib/mix/tasks/chatkit_room.ex

…ing answer

lib/cadet/chat/room.ex

geshuming

Is there any way to check if a room key is valid? Currently it seems that we assume that comment is either nil or VALID_KEY

lib/mix/tasks/chatkit_room.ex

geshuming · 2019-07-06T03:13:41Z

lib/cadet/assessments/assessments.ex

+
+        if answer.comment == "" or answer.comment == nil do
+          Room.create_rooms(submission, answer, user)
+        end


Actually, can this be placed where a submission occurs?

May I clarify what you mean? Do you mean when a submission is being finalised?

Yes. I think it is fine to leave the function here for now. But ultimately room creation should be done when a submission is submitted right?

Sure. I'll place it here then? Since this is where all submissions flow through, whether it's submitted early or submitted by the autograder. Quite ugly tho :/

lib/cadet/assessments/answer.ex

lib/cadet/chat/room.ex

lib/cadet/assessments/assessments.ex

lib/mix/tasks/chatkit_room.ex

rrtheonlyone

Sorry for spam, I just have some other comments here:

"admin" seems to be hardcoded for get_superuser_token -> possible to make it a constant/variable (minor nit)
It is interesting that you are tagging each user with their user_id when creating. This is the primary key in the database. This means that the user ids will just be ["1", "2", "3"] and so on.

I believe this is brittle and can lead to errors. Consider a scenario in which the import script adds half of the users and breaks after that. Or if there is a change to one/two users in between. We then have to manually go into chatkit and cleanup the ids. However, this is unlikely to happen as the User table does not usually change over the semester and remains constant (its more of a what if..)

I prefer to use the NUS ID instead to create Users. This is just a suggestion/concern. @geshuming @flxffy do see if you want to change or not!

geshuming · 2019-07-06T04:02:21Z

I prefer to use the NUS ID instead to create Users. This is just a suggestion/concern. @geshuming @flxffy do see if you want to change or not!

@rrtheonlyone It should be fine. Many other core components also use user_id. Normally user rows should be preserved, if a user drops out of the course for example we can just delete their authorization, and not the user row.

flxffy · 2019-07-06T13:13:21Z

Is there any way to check if a room key is valid? Currently it seems that we assume that comment is either nil or VALID_KEY

@geshuming The only way to verify a room id's validity is through a GET request to Chatkit to fetch room info. There's no documentation on how the room ids are being generated either.

flxffy · 2019-07-06T17:20:20Z

I prefer to use the NUS ID instead to create Users. This is just a suggestion/concern. @geshuming @flxffy do see if you want to change or not!

@rrtheonlyone I used user_id because it's a unique identifier and a compulsory field. There's no NOT NULL constraint on the nusnet_id column so I'm going with user_id instead!

flxffy · 2019-07-06T17:28:32Z

Tests have been added for room creation.

geshuming

Looks ok. I will log issues from this PR

flxffy and others added 24 commits June 16, 2019 22:17

Logic for jwt generation used for chatkit complete

6dfbb12

Merge branch 'master' of https://github.com/source-academy/cadet

6185ec0

/chat/token controller

4c5026b

Set up API for chat token generation

5e447cf

Documentation for ChatKit token generation

018690c

Changed chatkit user identifier to user.id from db

4691dac

Merge branch 'master' of https://github.com/source-academy/cadet

334231f

update readme with chatkit instructions

5cb5ddf

code readbility

80fcc0b

Added private functions in chat token generator

f27ae9e

fixed typo in superuser name

9a3243b

Main logic for script to create all users

472ebca

Script for creating all chatkit users

2cfeae2

Add comment related functions

8491f78

Storage of ChatKit room id. Logic complete with failsafes in thevent …

1c3e241

…of Chatkit's server failure. Untested.

Remove test file

9e66f58

Refactored chat.ex

fdea41c

Updated moduledoc for cadet.users.chat

fefc2a5

Fixed unsubmit test

6bf7608

Create chatkit room when submission is retrieved only if authorised

3e851e4

fixed answer controller test case clause error

325e417

Merge branch 'master' of https://github.com/source-academy/cadet

51a0c9b

Merge branch 'id_storage'

3604b39

grading_controller_test.ex to accept unchanged comment field for unsu…

574472e

…bmit

flxffy requested review from geshuming and jiachen247 July 2, 2019 15:23

geshuming reviewed Jul 2, 2019

View reviewed changes

geshuming requested changes Jul 3, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

lib/cadet/assessments/assessments.ex Outdated Show resolved Hide resolved

lib/cadet/assessments/assessments.ex Outdated Show resolved Hide resolved

lib/cadet/assessments/assessments.ex Outdated Show resolved Hide resolved

flxffy added 2 commits July 5, 2019 00:36

Script for creating rooms as failsafe

42aa925

Scheduled script to run daily at 1am

2b01b52

geshuming requested changes Jul 5, 2019

View reviewed changes

flxffy added 9 commits July 5, 2019 10:19

Changed import to alias

4a628d9

Changed grading_changeset and changeset to exclude comment when updat…

039e5fa

…ing answer

Optimise room creation when students access

e69e7f0

Changed when rooms are created

a9b44d5

Refactored room.ex

d864c68

Else clause

a996615

Grading controller

b3ba7a4

Fixed tests

78c41a9

Script only for submitted submissions

19474a9

rrtheonlyone reviewed Jul 6, 2019

View reviewed changes

lib/cadet/chat/room.ex Outdated Show resolved Hide resolved

geshuming requested changes Jul 6, 2019

View reviewed changes

lib/mix/tasks/chatkit_room.ex Outdated Show resolved Hide resolved

rrtheonlyone reviewed Jul 6, 2019

View reviewed changes

flxffy added 3 commits July 6, 2019 15:29

Room creation logging

ff5fbcf

Tests for room creation

e709d0c

Merge branch 'master' of https://github.com/source-academy/cadet

3dc73c2

flxffy added 5 commits July 6, 2019 22:33

Changed db query in ChatkitRoom

d5c1fdf

Minor fix

aa75e3f

Minor fix to cassettes

21de4ea

Fixed tests

05b830d

Minor fix

842e534

geshuming approved these changes Jul 7, 2019

View reviewed changes

geshuming merged commit 0007377 into source-academy:master Jul 7, 2019

Integration of Chatkit #406

Integration of Chatkit #406

Uh oh!

Conversation

flxffy commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2513

💛 - Coveralls

Uh oh!

geshuming left a comment

Choose a reason for hiding this comment

Uh oh!

flxffy commented Jul 2, 2019

Uh oh!

geshuming left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geshuming left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geshuming left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geshuming Jul 6, 2019

Choose a reason for hiding this comment

Uh oh!

flxffy Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geshuming Jul 6, 2019

Choose a reason for hiding this comment

Uh oh!

flxffy Jul 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rrtheonlyone left a comment

Choose a reason for hiding this comment

Uh oh!

geshuming commented Jul 6, 2019

Uh oh!

flxffy commented Jul 6, 2019

Uh oh!

flxffy commented Jul 6, 2019

Uh oh!

flxffy commented Jul 6, 2019

Uh oh!

geshuming left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flxffy commented Jul 2, 2019 •

edited

Loading

coveralls commented Jul 2, 2019 •

edited

Loading

geshuming left a comment •

edited

Loading

flxffy Jul 6, 2019 •

edited

Loading