-
Notifications
You must be signed in to change notification settings - Fork 78
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
Programming Question Audit Trail #7595
base: master
Are you sure you want to change the base?
Conversation
42b2875
to
1bbfd55
Compare
@@ -3,6 +3,11 @@ is_displayed = @submission.graded? || @submission.published? | |||
|
|||
json.isAnswersDisplayed is_displayed | |||
|
|||
json.user do | |||
json.name @submission.creator.name.upcase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we sending name in uppercase in the data?
FE can use CSS text-transform: uppercase;
if needed, and why should the name be uppercase in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think during implementation I thought it looked better or something.
Removed usage of .upcase
since it can be done in FE if need be
json.isCodaveri question.is_codaveri? | ||
json.liveFeedbackEnabled question.live_feedback_enabled? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you want to explicitly add ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically within the codebase and in ruby booleans are all decorated with ?
So i changed it as such to indicate that it is a boolean
where(submission_id: submission_id, question_id: question_id) | ||
|
||
@all_answers = answers.limit(MAX_ANSWERS_COUNT) | ||
def fetch_all_answers(submission_id, question_id, limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should make limit
specify the actual number (i.e. limit default is MAX_ANSWERS_COUNT
, but can be value can pass as 1, 5, 50, or -1 for all.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -33,7 +33,7 @@ def all_answers | |||
@question_index = question_index(question_id) | |||
@all_answers = Course::Assessment::Answer. | |||
unscope(:order). | |||
order(:created_at). | |||
order(:submitted_at). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we also want to display submissions in attempting state, what happens to answers with nil
for submitted_at
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm I dont think there is a way to reconcile that. But do you think its ok for us to order by created_at
and also send the created_at
time but present it to the user as submitted_at
(which is what was done previously
@@ -4,6 +4,24 @@ class Course::Statistics::AnswersController < Course::Statistics::Controller | |||
|
|||
MAX_ANSWERS_COUNT = 10 | |||
|
|||
def latest_attempt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this should be renamed as latest_answer
to communicate that we are retrieving an answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -22,7 +22,7 @@ def latest_attempt | |||
discussion_topic: { posts: :codaveri_feedback }).first | |||
end | |||
|
|||
def question_answer_details | |||
def attempts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly why is this attempts
not answers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempt aligns with the vocab used in frontend, since we have never exposed the concept of "answer" to users
@@ -76,17 +76,17 @@ def question_index(question_id) | |||
end | |||
|
|||
def fetch_all_answers(submission_id, question_id, limit) | |||
@all_answers = if limit | |||
@all_answers = if limit == -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see you've updated the query to adjust the limit
parameter to take in a number. Please squash the relevant commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
end | ||
|
||
def all_answers | ||
def all_attempts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment, all_answers
vs all_attempts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempt aligns with the vocab used in frontend, since we have never exposed the concept of "answer" to users
result = @programming_question.class.transaction do | ||
duplicate_and_replace_programming_question |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're rearranging items, please squash the commit with the respective commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
1bbfd55
to
2f05498
Compare
add parentId for: - programming auto grading - programming question
- include an option to add elipses beside the title to better indicate that accordion can be opened
- make new of new accordion ellipses feature
- add card to attempts dialog & all attempts page to display information and hold buttons - removed unused translations from AllAttempts.tsx - user info added for all attempts page to properly display name in the new card - links for going to submission page and attempts page shifted to be inside the card
- QuestionDetails -> Question - AllAnswerDetails -> Answer - previous naming was verbose and communicated the wrong idea of what information the type has - free up naming for use in details components
- submitted_at is more indicative of the actual data
- changed naming for files and added specific typing for latest answer - fix bug where if a question is added after submission, the page crashes
- name change from answers -> attempts - change limit from a constant backend value to a constant FE value, add function for limiting dynamically
- updated types for processed vs non processed data - removed unnecessary data from jbuilders - added parsing / processing of programming question data and linked to display
- lines removed is data that is no longer being rendered
- use update column instead of update to prevent validation error when duplicating question with deprecated programming language
- also add redicrect edit url to return when updating question, so that id can be used to revalidate question
- this situation can occur when question is added after submission
2f05498
to
2d65d34
Compare
Motivation
Old Behavior - When question is updated and regrading is triggered, there was no preservation of the:
This resulted in a dilemma when randomness in test cases (such as in machine learning) resulted in a student failing test cases that were previously passing
Details
course_assessment_question_programming
andcourse_assessment_answer_programming_auto_gradings