Add API specs for Controllers #41

indocomsoft · 2018-06-09T12:58:35Z

coveralls · 2018-06-09T13:00:20Z

Pull Request Test Coverage Report for Build 492

38 of 42 (90.48%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.0%) to 96.845%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/cadet_web/router.ex	0	4	0.0%

Totals
Change from base Build 488:	-1.0%
Covered Lines:	307
Relevant Lines:	317

💛 - Coveralls

remo5000 · 2018-06-11T02:41:12Z

Missions looks good so far, thanks for the quick PR!

For modularity of fetching data, will there be a GET request for individual missions implemented later on?

indocomsoft · 2018-06-11T02:52:19Z

Yea, I'm thinking of giving less information in the missions listing, and then y'all can request more information about specific mission later on.

Which fields do you think is necessary for the missions index listing?

remo5000 · 2018-06-11T03:23:24Z

We are thinking of segregating it into two kinds:

One for displaying information (i.e in the 'missions' page), and
The entirety of the Mission information (when doing the Assessment).

The former would then allow us to fetch the minimal information for all of the missions quickly, while the latter would be fast because we're fetching detailed data for a singular mission.

indocomsoft · 2018-06-11T06:01:19Z

Yup, I got that, I'm just wondering which fields do you require for displaying information in the missions page? (e.g. You don't need question, but I'm not sure which other fields you need or don't need)

I think this is better so as to minimise the data sent. You can then get these additional informations when you request detailed data for a singular mission.

remo5000 · 2018-06-11T10:21:08Z

@indocomsoft We've come up with a mock of the data types that we would be displaying for assessments...the data is more or less similar to the model I saw in the swagger, barring a few instances of renaming. Also, we try to make a distinction between the different assessment types so as to conveniently fetch data according to the category later on and be more verbose

https://gist.github.com/remo5000/cd6923c6fa004442bbc7c633718966fc

ning-y · 2018-06-12T02:35:52Z

lib/cadet_web/controllers/missions_controller.ex

      Mission:
        swagger_schema do
          properties do
+            order(:integer, "The order of showing the missions", required: true)


Just to keep you in the know, we'll probably sort by due date or mission ID on the frontend (perhaps providing a toggle for the user). Still, no harm having this field here for back compatibility with mission data, but just to let you know.

remo5000 · 2018-06-13T13:46:54Z

I think Submission and GradingInfo look good. Just to clarify, for GradingInfo:

Is this a display-centric structure, i.e it combines data to return something for the front-end (that can be viewed when grading a submission)?
If it is not, we could use it within Grading, to represent the grading of individual questions. I feel like there will be less repetition of data if we keep questionId instead of weight. (only caveat being the trouble of a foreign key).
How would weight be calculated? In fact, we should decide on individual question marks or keeping the maximumEXP

As for Grading, I have this relationship in mind:

has a Submission, Trainer, Student (both users), timestamp
has many GradingInfos(?) to store how a trainer grades each question (i.e GradingInfo is not just for display). I feel like there is a better way to make this more ordered, but I'm not sure.

Any thoughts @ningyuansg @indocomsoft @sreycodes ?

indocomsoft · 2018-06-16T09:42:12Z

I'm trying my best to create a display-centric structure -- let me know if you require more or less.
Right now @sreycodes and I are thinking of abolishing weight altogether, and instead grader would give xp for the whole submission (instead of individual question). This is assuming that there will be no graded MCQ. What do the front-end team think about this?

To be honest, the API structure that I made were rather display-centric so it doesn't quite correspond one-to-one to the backend models & relationships.

In case you're interested, here's the entity-relationship diagram of the Assessments context of backend. (----< means has_many, - - - > means belongs_to, and the text on the arrows is the field name represented by that relationship)

remo5000 · 2018-06-17T02:59:32Z

@indocomsoft that looks good, I think the only modification when adding Grading would be shifting the grader field.

As for how to structure it, I think we should come up with structures that are easy to represent and modify. The API can then provide an ad-hoc structure using a combination of the simpler internal models. e.g the display while one is grading will take information from that particular Submission, Grading and Question.

I second removing weight, but do you think we could make do with individual marks for each Question? The total can then be dynamically counted by summing them up (when sending to an API request). So if, for example, a trainer decides to add a new question, just the question (and the marks it holds) would have to be added. What do you think?

indocomsoft · 2018-06-17T11:36:21Z

@remo5000 not sure what you mean by "shifting the grader field". Could you elaborate more?

Yup, that's what I'm thinking. In such case, there's no need to create a separate table since the data needed have been contained in the models & contexts to be merged from the assessments branch. (#49 can be closed then).

As for removing weight, the backend team could certainly do that! It all depends on what workflow the frontend team is thinking of using. We can switch it to summing up total marks dynamically.

I will try to finish up my API spec for students submitting answers and graders giving comments and XP points by today so you and @ningyuansg can review the API tomorrow. If everything is okay, the frontend team can then write the necessary API hooks and the backend team can start implementing the controllers.

indocomsoft · 2018-06-18T01:03:47Z

Alright, this is pretty much done. Let me know what you think @remo5000 @ningyuansg

remo5000 · 2018-06-18T02:29:53Z

@indocomsoft By moving the grader field, I mean decoupling a Grading (done by a trainer) from a Submission (done by a student). The changes would be just shifting the fields related to grading from the Submission model to the Grading model. This would make the schema more organised because we are ensuring that each table holds specific data. It also allows for the possibility of one-to-many relationship in the future (e.g for multiple people to grade a submission or to keep track of all the grading for a particular submission). I'll close the PR anyway, thanks for the speedy API specs 👍

indocomsoft · 2018-06-18T02:54:37Z

@remo5000 Ok, I see what you mean now.

I guess for now we are still gonna have a Submission model with both grader and student field since the assessments branch is long overdue to be merged. Perhaps you could open an issue to refactor this in the future, once we are done implementing the basic functionalities.

What do you think? @tuesmiddt @sreycodes

remo5000 · 2018-06-18T03:28:06Z

lib/cadet_web/controllers/missions_controller.ex

+  swagger_path :questions do
+    get("/missions/{missionId}/questions")
+
+    summary("Get questions contained inside a mission. Response is either `mcq` or `programming`")


Does this mean that the response can contain (exclusive) either of the two? Right now the structure appears to be two arrays, one for programming and the other for mcq

Yup, it's exclusive. Cos the swagger library we're using is still on OpenAPI 2.0 which doesn't allow oneOf 😞

remo5000 · 2018-06-18T03:44:17Z

lib/cadet_web/controllers/missions_controller.ex

+        swagger_schema do
+          properties do
+            questionId(:integer, "question id", required: true)
+            content(:string, "the question content", required: true)


Is this provided in Markdown? Or is it just the content/prompt of the question? @indocomsoft

It's just the prompt of the question, but we can store markdown, sure since it's just a string -- that depends on the frontend team decision

ning-y · 2018-06-18T03:37:23Z

lib/cadet_web/controllers/missions_controller.ex

+  end
+
+  swagger_path :questions do
+    get("/missions/{missionId}/questions")


I don't think we need an API call for each question. Rather, the questions for a particular assessment can be provided with the API call for that assessment as an array. This helps us bind the right actions to the onClicks for next, prev, and submit (and when to render these buttons) of the workspace.

i.e. the mission model can supply us with questions as an array property.

It looks like one query to /missions/{missionId}/questions returns all the questions. However, you would then need to hit the /answer/{type}/id endpoint once per question to get the user's previous submission. Since we need to be able to refresh questions individually, I'm thinking maybe an alternative is to have /answer/{type}?id={id1}?id={id2} return a JSON of

{ id1: { type: "mcq", answer: 4 }, id2: { type: "programming", answer: "console.log('hello world!');" }, ... }

+1, though I'd prefer to not have to specify what type of question (MCQ or programming) in the query string.

You shouldn't need to specify the question type in the query. I guess it's not even necessary to include the type in the response. You would then need to check against the locally stored collection of questions to determine question type (and thus how to interpret the value).

Alternatively, for simplicity, we can represent MCQ answers as a string (specifically a character). So something like:

{ id1: "d", // or "4" id2: "console.log('hello world!');" }

We can then also unify MCQ/programming submission apis into say GET|POST /question/answer?id={id1} (must validate only 1 id is supplied for POST) that returns an optional correctness response.

@indocomsoft Thoughts?

@tuesmiddt Yeah, sorry the /answer/* endpoints were my very early plans -- we should be able to unify the 2 types.

As for MCQ answer, I was thinking to maybe tag each MCQ choice associated with an MCQ question with a pseudo-id that will be used to identify them uniquely (combination of questionId + choice pseudo-id). Doesn't sound very nice to me -- let me know if any of you have any suggestion. @remo5000 @sreycodes @rrtheonlyone

@ningyuansg would you then rather have the questions themselves embedded in the get("/missions/{missionId}") endpoint? That way we can just merge the two.

ning-y · 2018-06-18T03:38:36Z

lib/cadet_web/controllers/missions_controller.ex

+            mission_pdf(:string, "The URL to the mission pdf")
+          end
+        end,
+      Questions:


Can we have a field in questions that is either 'MCQ' or 'Programming', or something to that effect? On our end, it's safer to check the string value of this property rather than something like if (question.choices === undefined).

Sure. So the json will be either

{ "type": "mcq", "mcq": [ { "questionId": 1, "content": "test", "choices": [{"content": "a", "hint": "asd"}]}] } ] }

or

{ "type": "programming", "programming": [ { "questionId": 2, "content": "kjnfb", "solution_template": "sjdfkb", "library": { "chapter": 1, "globals": [], "externals": [], "files": [] } } ] }

Sounds good?

ning-y · 2018-06-18T03:39:25Z

lib/cadet_web/controllers/missions_controller.ex

@@ -0,0 +1,190 @@
+defmodule CadetWeb.MissionsController do


Do these missions include paths, contests, and sidequests? In that case, can we rename this as Assessments?

ning-y · 2018-06-18T03:52:24Z

lib/cadet_web/router.ex

+      post("/programming", ProgrammingAnswerController, :submit)
+      get("/mcq/:id", MCQAnswerController, :show)
+      post("/mcq", MCQAnswerController, :submit)
+    end


As I understand it, question IDs are only unique within a mission, so this needs a mission ID somewhere.

Also, can we have one path for all questions, no matter the type (programming or mcq)? In that case, you could nest this under the current /missions path as the POST and GET at /missions/{missionId}/{questionId}.

question IDs should be unique across all but anyway this AnswerController needs a rework to unify the two kinds of questions anyway.

remo5000 · 2018-06-18T05:22:29Z

@indocomsoft My bad, could have communicated my idea better. Is it possible to change the /auth API specs here to reflect #48 (since it's going to be merged soon)?

ning-y · 2018-06-18T06:34:41Z

lib/cadet_web/router.ex

    post("/auth/logout", AuthController, :logout)
  end

  # Authenticated Pages


We need a path to get user information, such as username and (later on) progress on the story. Perhaps a /user path?

I think we will probably need a full users controller. I can see the need to implement more features as we go along (notification preferences, display picture, whatnot)

tuesmiddt · 2018-06-20T06:36:44Z

lib/cadet_web/controllers/answer/programming_answer_controller.ex

+          properties do
+            questionId(:integer, "The question id", required: true)
+
+            submitted(:boolean, "Whether this is the final answer", default: false)


Do we really need a submitted flag or can we just take the most recent submission as final?

We need it cos students might wanna save them as "draft" -- once submitted, it's locked from being edited and the TA can start grading

indocomsoft · 2018-06-23T10:07:49Z

lib/cadet_web/controllers/missions_controller.ex

+    response(401, "Unauthorised")
+  end
+
+  swagger_path :open do


Remove this

tuesmiddt · 2018-06-24T04:24:23Z

Updated to reflect results of yesterday's discussion.

I renamed the submit endpoint to POST /assessments/question/{questionId}/submit.

GET /grading should need further changes depending on what other information front-end wants to display on the listing page (student name, discussion group, etc).

tuesmiddt · 2018-06-24T16:29:18Z

@ningyuansg @remo5000 The objects in the array returned by the GET /assessments endpoint no longer contain an order prop. I thought it's unnecessary since it's the array. Lmk if you do need it for other purposes.

indocomsoft · 2018-06-24T16:30:44Z

lib/cadet_web/controllers/assessments_controller.ex

+        end,
+      Assessment:
+        swagger_schema do
+          properties do


I think we should add type here just to standardise with the frontend data structure

I didn't add this cos AssessmentCategory was in a different colour in the .ts schema. Need to get my eyes checked

Lmao at least the commit message isn't I'm blind

indocomsoft · 2018-06-24T16:32:37Z

lib/cadet_web/controllers/answer_controller.ex

+    post("/assessments/question/{questionId}/submit")
+
+    summary("Submit an answer to a question")
+


I think you should add description("For MCQ, answer contains choice_id. For programming question, this is a string containing the code.") or sth to that effect

indocomsoft · 2018-06-24T17:12:28Z

LGTM

remo5000 · 2018-07-04T04:21:03Z

lib/cadet_web/controllers/assessments_controller.ex

+            solution(:integer, "Solution to a mcq question if it belongs to path assessment")
+
+            answer(
+              :string_or_integer,


@indocomsoft I was looking at the Grading specs and saw this. An issue with swagger perhaps?

Also, I think questionType and questionId should be changed to snake_case. I use camelCase for Typescript so that's probably where it got mixed in

* Add ProgrammingSubmissionController API spec * API docs for both MCQ and Programming Submission * Reorganise files * Add missions controller index API Spec * Rename submission to answer and add :show method * Add more missions controller spec * Added changes so far * Add :show to GradingController and various fixups * Fix Library according to #43 * Complete GradingController api spec * Unify marks and comments * Amend assessments controller * Remove missions controller * Minor fix * I'm blind * Update Answer controller spec * Add answer controller test file * Update grading controller spec * Rename answers controller file * Change submit method to POST * Split Assessment swagger model into two models * Minor changes to add missing description/field

indocomsoft changed the title ~~WIP: Add API specs for Submission & Grading Controllers~~ WIP: Add API specs for Controllers Jun 9, 2018

indocomsoft force-pushed the api-spec branch from d54d34c to 410a07c Compare June 11, 2018 14:38

ning-y reviewed Jun 12, 2018

View reviewed changes

remo5000 mentioned this pull request Jun 14, 2018

[WIP] Implement Grading structure #49

Closed

6 tasks

indocomsoft added 10 commits June 17, 2018 21:28

Add ProgrammingSubmissionController API spec

d90f196

API docs for both MCQ and Programming Submission

cf736a7

Reorganise files

032f924

Add missions controller index API Spec

3ba228a

Rename submission to answer and add :show method

7c7cb2e

Add more missions controller spec

3e3434e

Added changes so far

90c741d

Add :show to GradingController and various fixups

a0bd9b5

Fix Library according to #43

570573d

Complete GradingController api spec

37dd432

indocomsoft force-pushed the api-spec branch from 2b69a20 to 37dd432 Compare June 17, 2018 13:29

Unify marks and comments

95b7851

indocomsoft changed the title ~~WIP: Add API specs for Controllers~~ Add API specs for Controllers Jun 18, 2018

remo5000 reviewed Jun 18, 2018

View reviewed changes

ning-y requested changes Jun 18, 2018

View reviewed changes

tuesmiddt reviewed Jun 20, 2018

View reviewed changes

indocomsoft commented Jun 23, 2018

View reviewed changes

indocomsoft and others added 10 commits June 23, 2018 22:49

Merge branch 'master' into api-spec

e6382dc

Amend assessments controller

1b2150a

Remove missions controller

05708fc

Minor fix

acf75b9

I'm blind

a434634

Update Answer controller spec

bc79827

Add answer controller test file

af34180

Update grading controller spec

5190e14

Rename answers controller file

4cc6799

Change submit method to POST

c60414c

ning-y approved these changes Jun 24, 2018

View reviewed changes

tuesmiddt added 2 commits June 25, 2018 00:18

Split Assessment swagger model into two models

edd0ed2

Merge branch 'master' into api-spec

51dc16c

indocomsoft commented Jun 24, 2018

View reviewed changes

Minor changes to add missing description/field

7f247e8

indocomsoft merged commit 72a8a1e into master Jun 24, 2018

indocomsoft deleted the api-spec branch June 24, 2018 17:12

remo5000 reviewed Jul 4, 2018

View reviewed changes

		post("/assessments/question/{questionId}/submit")

		summary("Submit an answer to a question")

Add API specs for Controllers #41

Add API specs for Controllers #41

Uh oh!

Conversation

indocomsoft commented Jun 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 492

💛 - Coveralls

Uh oh!

remo5000 commented Jun 11, 2018

Uh oh!

indocomsoft commented Jun 11, 2018

Uh oh!

remo5000 commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indocomsoft commented Jun 11, 2018

Uh oh!

remo5000 commented Jun 11, 2018 • edited by ning-y Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remo5000 commented Jun 13, 2018

Uh oh!

indocomsoft commented Jun 16, 2018

Uh oh!

remo5000 commented Jun 17, 2018

Uh oh!

indocomsoft commented Jun 17, 2018

Uh oh!

indocomsoft commented Jun 18, 2018

Uh oh!

remo5000 commented Jun 18, 2018

Uh oh!

indocomsoft commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuesmiddt Jun 20, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

tuesmiddt Jun 20, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remo5000 commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indocomsoft commented Jun 9, 2018 •

edited

Loading

coveralls commented Jun 9, 2018 •

edited

Loading

remo5000 commented Jun 11, 2018 •

edited

Loading

remo5000 commented Jun 11, 2018 •

edited by ning-y

Loading

indocomsoft commented Jun 18, 2018 •

edited

Loading

tuesmiddt Jun 20, 2018 •

edited

Loading

tuesmiddt Jun 20, 2018 •

edited

Loading

remo5000 commented Jun 18, 2018 •

edited

Loading

indocomsoft Jun 24, 2018 •

edited

Loading

indocomsoft Jun 24, 2018 •

edited

Loading