Skip to content
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

Refactor Live Feedback Settings for Assessment Edit and Submission Form #7553

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bivanalhar
Copy link
Contributor

@bivanalhar bivanalhar commented Sep 14, 2024

Background

It's been found out that recently, only Manager/Owner can access the Assessment Edit Page, and that accessing this page while having Codaveri Component disabled caused unexpected error. Upon further inspection, it's found out that the attempt to fetch the Codaveri Settings for Assessment, which is being done while accessing the Assessment Edit Page, calls out the controller in which only Manager/Owner has the ability to do so.

Fix

Upon this inspection, we decided to do several fixes, as well as the further enhancement towards the Assessment Edit Page, as follows:

  • decouple the logic for fetching codaveri settings and toggling on/off codaveri settings for assessment from codaveri settings controller. We placed the logic for these 2 inside the Assessment controller, in which Teaching Assistant also has right to access the controller
  • we do not return any programming questions (regardless of whether some of them is support-ible by codaveri) and disable Get Help in the assessment level if the Codaveri Component is disabled for the course, and give info to user to enable the codaveri component first
Screenshot 2024-09-14 at 10 38 11 PM

Miscellaneous

Within this PR, we also do the refactor of Submission Form, not to render Get Help button if Codaveri Component is disabled

@bivanalhar bivanalhar force-pushed the bivan/codaveri-assessment-fix branch 2 times, most recently from c6bf788 to 6ff1bbb Compare September 14, 2024 00:31
@bivanalhar bivanalhar changed the title Refactor Live Feedback Settings for Assessment Edit Refactor Live Feedback Settings for Assessment Edit and Submission Form Sep 14, 2024
isCourseCodaveriDisabled: {
id: 'course.assessment.AssessmentForm.isCourseCodaveriDisabled',
defaultMessage:
'Please contact the Course Administrator to enable Codaveri Evaluator\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no terminology "Course Administrator", update and follow existing example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -30,6 +32,8 @@ const NonAutogradedProgrammingActionButtonsRow: FC<Props> = (props) => {
const question = questions[questionId];
const { viewHistory } = question;

const { isCodaveriEnabled } = assessment;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my opinion, this should be isCourseCodaveriEnabled to make it clear that this is the course attribute, but since this comes from a very old commit, we can leave this for now, since there are already many changes in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

describe '#live_feedback_settings' do
render_views
let(:assessment) { create(:assessment, course: course) }
let(:lang_valid_for_codaveri) { Coursemology::Polyglot::Language::Python::Python3Point12.instance }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a TODO that this should be updated when codaveri_language_whitelist changes in coursemology2/app/services/codaveri_async_api_service.rb. Eventually we will have a dynamic list pulled from Codaveri during production, and mock a list during tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

config/routes.rb Outdated
get :requirements, on: :member
get :statistics, on: :member
get :monitoring, on: :member
patch :update_live_feedback_settings, on: :member
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET and PATCH are different HTTP request types, and should use the same route :live_feedback_settings, don't make a new route. See other routes for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 66 to 67
tabs: [],
categories: [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why tabs / categories are set to empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually we couldn't care less about the tabs and categories; this is for assessmentEdit only.. However, the input of tabs and categories are required to be stored in redux, and hence it's technically can be set to anything. I just put empty array there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up refactoring the action call for this (so now tabs and categories can be undefined, and in that case, we don't update tabs and categories on redux)

@@ -15,11 +16,17 @@ import CodaveriSettingsChip from '../CodaveriSettingsChip';
interface LiveFeedbackToggleButtonProps {
assessmentIds: number[];
for: string;
hideChipIndicator?: boolean;
forSpecificAssessment?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

booleans please follow is has prefix convention where possible instead of custom naming, unless absolutely necessary.
isSpecificAssessment will work fine here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


updateLiveFeedbackSettings(assessmentId, params) {
return this.client.patch(
`${this.#urlPrefix}/${assessmentId}/update_live_feedback_settings`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this route to follow updated routes.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

- simplify logic for live feedback toggle in Assessment Edit
- remove "Get Help" button inside SubmissionForm, regardless of whether question has live feedback enabled or not
- disable Live Feedback Toggle inside Assessment Edit
- Not returning any programming questions for live feedback inside assessment edit
- Add Info Label when course has its codaveri disabled, and when no programming questions supportible by codaveri is present
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants