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

Assessments & Grading Workspace: Add folders mode #2765

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

Conversation

accountexeregister
Copy link
Member

@accountexeregister accountexeregister commented Feb 7, 2024

Description

To be merged with Grader #319 and Backend #1110

  • Adds folder support to Assessment & Grading workspaces
  • Folder mode is only supported for Source >= S2
  • Support for test cases to be ran on the existing Grader [See grader repo PR]
  • Support for test cases to be ran locally

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

Note: This PR might break "Teams" functionality. The shouldDisableSaveButton() function on line 805 in src\commons\assessmentWorkspace\AssessmentWorkspace.tsx causes the save button to always be disabled in Assessments. Hence we had to comment it out.

How to test

  1. Enter any Assessment and enable folder mode
  2. Question files from future questions that do not have a submission yet should not appear in the Folder Mode
  3. All question files should not be editable except for the current question
  4. Testcases should have an entrypoint file for the current question (targetting the current question)
  5. After a submission is made, the Grading workspace should be able to view the submission and also run all files in Folder Mode
  6. If testcases are present, and auto grading is enabled, it should auto grade using Grader

Checklist

  • I have tested this code
  • I have updated the documentation

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7817133053

  • -4 of 10 (60.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.004%) to 37.198%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/commons/assessmentWorkspace/AssessmentWorkspace.tsx 6 10 60.0%
Files with Coverage Reduction New Missed Lines %
src/commons/assessmentWorkspace/AssessmentWorkspace.tsx 1 69.71%
Totals Coverage Status
Change from base Build 7812213802: -0.004%
Covered Lines: 5729
Relevant Lines: 14468

💛 - Coveralls

@RichDom2185
Copy link
Member

Is this PR ready or still in draft state?

@accountexeregister
Copy link
Member Author

It is PR ready. The draft means we plan to expand on it.

@accountexeregister
Copy link
Member Author

Update: Sorry, it is not PR ready yet.

@RichDom2185 RichDom2185 marked this pull request as draft February 8, 2024 17:14
@martin-henz
Copy link
Member

@accountexeregister what is the status of this PR?

@accountexeregister
Copy link
Member Author

Sorry prof, I just saw this message. We will still have to work on the backend before integrating it fully.

@Tkaixiang Tkaixiang changed the title Folders: (Draft) Add read-only workspace Assessments & Grading Workspace: Add folders mode Apr 15, 2024
@RichDom2185 RichDom2185 marked this pull request as ready for review April 15, 2024 16:14
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

Hi, thanks for working on this, I did a quick run through and have the following comments, thanks!

Comment on lines +6 to +12
/**
* @prop fileMode an integer for the mode of the file where
* 0 = read-only and 1 = read-write.
*/
type ControlBarFileModeButtonProps = {
fileMode: number | null;
};
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using an integer when we can use an enum?

Comment on lines +14 to +19
export const ControlBarFileModeButton: React.FC<ControlBarFileModeButtonProps> = ({ fileMode }) => {
if (fileMode === 0) {
return <ControlButton label={'Read-Only'} icon={IconNames.LOCK} isDisabled />;
}
return <ControlButton label={'Read-Write'} icon={IconNames.UNLOCK} isDisabled />;
};
Copy link
Member

Choose a reason for hiding this comment

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

If the button is always disabled, is a button the most appropriate?

Comment on lines +24 to +30

/*
? props.readOnly
? 'Evaluation is disabled in read-only mode'
: '...or press shift-enter in the editor'
: 'Open a file to evaluate the program with the file as the entrypoint';
*/
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed? If not, please remove the commented code.

Comment on lines +11 to 30
isSupportedSource: boolean;
toggleFolderMode: () => void;
};

export const ControlBarToggleFolderModeButton: React.FC<Props> = ({
isFolderModeEnabled,
isSessionActive,
isPersistenceActive,
isSupportedSource,
toggleFolderMode
}) => {
const tooltipContent = isSessionActive
? 'Currently unsupported while a collaborative session is active'
: isPersistenceActive
? 'Currently unsupported while a persistence method is active'
: !isSupportedSource
? 'Folder mode is not available for this version of Source'
: `${isFolderModeEnabled ? 'Disable' : 'Enable'} Folder mode`;
return (
<Tooltip content={tooltipContent}>
Copy link
Member

Choose a reason for hiding this comment

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

I'd argue that if the language does not support it, the button should just be hidden (not disabled), like on the Playground.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think?

@@ -24,6 +25,10 @@ const EditorTab: React.FC<Props> = ({ filePath, isActive, setActive, remove }) =
})}
onClick={setActive}
>
{' '}
Copy link
Member

Choose a reason for hiding this comment

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

Is this space intentional?

: editorTab
);

if (isEqual(editorTabs, newEditorTabs)) {
Copy link
Member

Choose a reason for hiding this comment

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

Equality does not work properly with drafts. See https://immerjs.github.io/immer/pitfalls/#drafts-arent-referentially-equal

Comment on lines +809 to +815
return {
...state,
[workspaceLocation]: {
...state[workspaceLocation],
editorTabs: newEditorTabs
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Try to use mutation instead of returning a new object.

);

if (isEqual(editorTabs, newEditorTabs)) {
return state;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this very expensive computation, just read the old state (O(1) time), and read its readOnly old value (also O(1) time). Then compare it with the payload. If they are equal, return the old state. If they are not equal, compute the new state and return it.

QuestionTypes,
Testcase
} from '../assessment/AssessmentTypes';
import { ControlBarProps } from '../controlBar/ControlBar';
import { ControlBarChapterSelect } from '../controlBar/ControlBarChapterSelect';
import { ControlBarClearButton } from '../controlBar/ControlBarClearButton';
import { ControlBarEvalButton } from '../controlBar/ControlBarEvalButton';
// import { ControlBarFileModeButton } from '../controlBar/ControlBarFileModeButton';
Copy link
Member

Choose a reason for hiding this comment

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

Commented code should be removed in unused.

}, []);
setIsEditable(false);
dispatch(updateTabReadOnly(workspaceLocation, activeEditorTabIndex, true));
}, [activeEditorTabIndex, editorTabs, questionId]);
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the lint warnings.

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.

6 participants