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

Lecture: Allow instructors to remove solution slides when processing units #6895

Merged
merged 12 commits into from
Jul 29, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@

List<PDDocument> documentUnits = pdfSplitter.split(document);
pdDocumentInformation.setTitle(lectureUnit.unitName());
if (lectureUnitInformationDTO.removeBreakSlides()) {
removeBreakSlides(documentUnits.get(0));
if (lectureUnitInformationDTO.removeBreakSlides() || lectureUnitInformationDTO.removeSolutionSlides()) {
removeBreakOrSolutionSlides(documentUnits.get(0), lectureUnitInformationDTO.removeBreakSlides(), lectureUnitInformationDTO.removeSolutionSlides());
}
documentUnits.get(0).setDocumentInformation(pdDocumentInformation);
documentUnits.get(0).save(outputStream);
Expand All @@ -96,28 +96,30 @@
}

/**
* Removes the break slides from the given document.
* Removes the break slides or solution slides from the given document.
*
* @param document document to remove break slides from
*/
private void removeBreakSlides(PDDocument document) {
private void removeBreakOrSolutionSlides(PDDocument document, boolean removeBreakSlides, boolean removeSolutionSlides) {

try {
PDFTextStripper pdfTextStripper = new PDFTextStripper();
Splitter pdfSplitter = new Splitter();
List<PDDocument> pages = pdfSplitter.split(document);
Iterator<PDDocument> iterator = pages.listIterator();

int index = 0;
while (iterator.hasNext()) {
PDDocument currentPage = iterator.next();
// Uses a decrementing loop (starting from the last index) to ensure that the
// index values are adjusted correctly when removing pages.
for (int index = pages.size() - 1; index >= 0; index--) {
PDDocument currentPage = pages.get(index);
String slideText = pdfTextStripper.getText(currentPage);
if (isBreakSlide(slideText)) {

if (isBreakSlide(slideText) && removeBreakSlides) {
lennart-keller marked this conversation as resolved.
Show resolved Hide resolved
document.removePage(index);

Check warning on line 117 in src/main/java/de/tum/in/www1/artemis/service/LectureUnitProcessingService.java

View check run for this annotation

Teamscale / teamscale-findings

src/main/java/de/tum/in/www1/artemis/service/LectureUnitProcessingService.java#L117

Statements in Conditional Expression are equal https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=4550D7B64DFB214A8B491E90EB4C70F2
}
else if (isSolutionSlide(slideText) && removeSolutionSlides) {
document.removePage(index);

Check warning on line 120 in src/main/java/de/tum/in/www1/artemis/service/LectureUnitProcessingService.java

View check run for this annotation

Teamscale / teamscale-findings

src/main/java/de/tum/in/www1/artemis/service/LectureUnitProcessingService.java#L120

Statements in Conditional Expression are equal https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=B332CE47A6FFAEFE33DC5ABAFDCA8AD7
break;
}
currentPage.close(); // make sure to close the document
index++;
}
}
catch (IOException e) {
Expand All @@ -130,6 +132,10 @@
return slideText.contains("Break") || slideText.contains("Pause");
}

private boolean isSolutionSlide(String slideText) {
return slideText.contains("Example solution") || slideText.contains("Example solution (with comments)") || slideText.contains("Example solution: model");
krusche marked this conversation as resolved.
Show resolved Hide resolved
pellumbbaboci marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Prepare information of split units for client
*
Expand All @@ -147,8 +153,8 @@
List<LectureUnitSplitDTO> units = unitsDocumentMap.values().stream()
.map(lectureUnitSplit -> new LectureUnitSplitDTO(lectureUnitSplit.unitName, ZonedDateTime.now(), lectureUnitSplit.startPage, lectureUnitSplit.endPage))
.toList();
// return units information, maximum number of pages and by default remove break slides is false
return new LectureUnitInformationDTO(units, numberOfPages, false);
// return units information, maximum number of pages and by default remove break slides and remove solution slides are false
return new LectureUnitInformationDTO(units, numberOfPages, false, false);
}
catch (IOException e) {
log.error("Error while preparing the map with information", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@

import java.util.List;

public record LectureUnitInformationDTO(List<LectureUnitSplitDTO> units, int numberOfPages, boolean removeBreakSlides) {
public record LectureUnitInformationDTO(List<LectureUnitSplitDTO> units, int numberOfPages, boolean removeBreakSlides, boolean removeSolutionSlides) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,22 @@ <h2 id="page-heading">
<input class="form-check-input" type="checkbox" name="processUnit" [disabled]="units.length < 1" (change)="onSelectRemoveBreakSlides()" />
<span class="px-1">{{ 'artemisApp.attachmentUnit.createAttachmentUnits.removeBreakSlide' | artemisTranslate }}</span>
<fa-icon [icon]="faQuestionCircle" [ngbTooltip]="'artemisApp.attachmentUnit.createAttachmentUnits.removeBreakSlideTooltip' | artemisTranslate"></fa-icon>
<div *ngIf="removeBreakSlides" class="alert alert-warning mt-3 ml-3">
<span>
{{ 'artemisApp.attachmentUnit.createAttachmentUnits.removeBreakSlideInfo' | artemisTranslate }}
</span>
</div>
</div>

<div class="mt-3 mb-3">
<input class="form-check-input" type="checkbox" name="processUnit" [disabled]="units.length < 1" (change)="onSelectRemoveSolutionSlides()" />
<span class="px-1">{{ 'artemisApp.attachmentUnit.createAttachmentUnits.removeSolutionSlide' | artemisTranslate }}</span>
<fa-icon [icon]="faQuestionCircle" [ngbTooltip]="'artemisApp.attachmentUnit.createAttachmentUnits.removeSolutionSlideTooltip' | artemisTranslate"></fa-icon>
<div *ngIf="removeSolutionSlides" class="alert alert-warning mt-3 ml-3">
<span>
{{ 'artemisApp.attachmentUnit.createAttachmentUnits.removeSolutionSlideInfo' | artemisTranslate }}
</span>
</div>
</div>

<div *ngIf="units && units.length < 1" class="alert alert-danger mt-3">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type LectureUnitInformationDTO = {
units: LectureUnitDTOS[];
numberOfPages: number;
removeBreakSlides: boolean;
removeSolutionSlides: boolean;
};

@Component({
Expand Down Expand Up @@ -48,6 +49,7 @@ export class AttachmentUnitsComponent implements OnInit {
fileName: string;
invalidUnitTableMessage?: string;
removeBreakSlides: boolean;
removeSolutionSlides: boolean;

constructor(
private activatedRoute: ActivatedRoute,
Expand All @@ -67,6 +69,7 @@ export class AttachmentUnitsComponent implements OnInit {

ngOnInit(): void {
this.removeBreakSlides = false;
this.removeSolutionSlides = false;
this.isLoading = true;
this.isProcessingMode = true;

Expand Down Expand Up @@ -95,7 +98,12 @@ export class AttachmentUnitsComponent implements OnInit {
createAttachmentUnits(): void {
if (this.validUnitInformation()) {
this.isLoading = true;
const lectureUnitInformationDTOObj: LectureUnitInformationDTO = { units: this.units, numberOfPages: this.numberOfPages, removeBreakSlides: this.removeBreakSlides };
const lectureUnitInformationDTOObj: LectureUnitInformationDTO = {
units: this.units,
numberOfPages: this.numberOfPages,
removeBreakSlides: this.removeBreakSlides,
removeSolutionSlides: this.removeSolutionSlides,
};
Strohgelaender marked this conversation as resolved.
Show resolved Hide resolved
const formData: FormData = new FormData();
formData.append('file', this.file);
formData.append('lectureUnitInformationDTO', objectToJsonBlob(lectureUnitInformationDTOObj));
Expand Down Expand Up @@ -199,4 +207,8 @@ export class AttachmentUnitsComponent implements OnInit {
onSelectRemoveBreakSlides() {
this.removeBreakSlides = !this.removeBreakSlides;
}

onSelectRemoveSolutionSlides() {
this.removeSolutionSlides = !this.removeSolutionSlides;
}
}
4 changes: 4 additions & 0 deletions src/main/webapp/i18n/de/lectureUnit.json
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@
"processAutomatically": "Automatische Einheitenverarbeitung",
"removeBreakSlide": "Entferne Pausenfolien",
"removeBreakSlideTooltip": "Folien, die 'Break' enthalten, werden entfernt.",
"removeBreakSlideInfo": "Alle Pausenfolien werden aus allen Einheiten entfernt.",
"removeSolutionSlide": "Folien mit Beispiellösungen entfernen",
"removeSolutionSlideTooltip": "Wenn die Folie das Wort \"Example solution\" enthält, wird die Folie entfernt.",
pellumbbaboci marked this conversation as resolved.
Show resolved Hide resolved
"removeSolutionSlideInfo": "Alle Folien mit Beispiellösungen werden entfernt.",
"validation": {
"invalidPages": "Die Startseite oder die letzte Seite von {{ unitName }} sind falsch eingestellt.",
"empty": {
Expand Down
6 changes: 5 additions & 1 deletion src/main/webapp/i18n/en/lectureUnit.json
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,12 @@
"lectureFileToolTip": "Please select to enable the automatic separation of lecture units.",
"processUnits": "Process units",
"processAutomatically": "Automatic unit processing",
"removeBreakSlide": "Remove Break slide",
"removeBreakSlide": "Remove Break slides",
"removeBreakSlideTooltip": "If the slide contains the word 'Break' it will remove the slide from the unit.",
"removeBreakSlideInfo": "All Break slides will be removed from all units.",
"removeSolutionSlide": "Remove Example Solution slides",
"removeSolutionSlideTooltip": "If the slide contains the word 'Example solution' it will remove the slide from the unit.",
pellumbbaboci marked this conversation as resolved.
Show resolved Hide resolved
"removeSolutionSlideInfo": "All Example solution slides will be removed from all units.",
"validation": {
"invalidPages": "The start page or end page of {{ unitName }} are set incorrectly.",
"empty": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
userUtilService.addUsers(TEST_PREFIX, 1, 1, 0, 1);
this.lecture1 = lectureUtilService.createCourseWithLecture(true);
List<LectureUnitSplitDTO> units = new ArrayList<>();
this.lectureUnitSplits = new LectureUnitInformationDTO(units, 1, true);
this.lectureUnitSplits = new LectureUnitInformationDTO(units, 1, true, false);
// Add users that are not in the course
userUtilService.createAndSaveUser(TEST_PREFIX + "student42");
userUtilService.createAndSaveUser(TEST_PREFIX + "tutor42");
Expand Down Expand Up @@ -106,7 +106,7 @@
assertThat(lectureUnitSplitInfo.units()).hasSize(2);
assertThat(lectureUnitSplitInfo.numberOfPages()).isEqualTo(20);

lectureUnitSplitInfo = new LectureUnitInformationDTO(lectureUnitSplitInfo.units(), lectureUnitSplitInfo.numberOfPages(), false);
lectureUnitSplitInfo = new LectureUnitInformationDTO(lectureUnitSplitInfo.units(), lectureUnitSplitInfo.numberOfPages(), false, false);

List<AttachmentUnit> attachmentUnits = List.of(request.postWithMultipartFile("/api/lectures/" + lecture1.getId() + "/attachment-units/split", lectureUnitSplitInfo,
"lectureUnitInformationDTO", filePart, AttachmentUnit[].class, HttpStatus.OK));
Expand Down Expand Up @@ -153,7 +153,7 @@
LectureUnitInformationDTO.class, HttpStatus.OK);
assertThat(lectureUnitSplitInfo.units()).hasSize(2);
assertThat(lectureUnitSplitInfo.numberOfPages()).isEqualTo(20);
lectureUnitSplitInfo = new LectureUnitInformationDTO(lectureUnitSplitInfo.units(), lectureUnitSplitInfo.numberOfPages(), true);
lectureUnitSplitInfo = new LectureUnitInformationDTO(lectureUnitSplitInfo.units(), lectureUnitSplitInfo.numberOfPages(), true, false);

List<AttachmentUnit> attachmentUnits = List.of(request.postWithMultipartFile("/api/lectures/" + lecture1.getId() + "/attachment-units/split", lectureUnitSplitInfo,
"lectureUnitInformationDTO", filePart, AttachmentUnit[].class, HttpStatus.OK));
Expand All @@ -166,14 +166,57 @@
byte[] fileBytes = request.get(attachmentPath, HttpStatus.OK, byte[].class);

try (PDDocument document = PDDocument.load(fileBytes)) {
// 12 is the number of pages for the first unit without the break slide
assertThat(document.getNumberOfPages()).isEqualTo(12);
// 6 is the number of pages for the first unit without the break slides
assertThat(document.getNumberOfPages()).isEqualTo(6);
document.close();
}
assertThat(attachmentUnitList).hasSize(2);
assertThat(attachmentUnitList).isEqualTo(attachmentUnits);
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void splitLectureFile_asInstructor_shouldRemoveSolutionSlides_and_removeBreakSlides() throws Exception {

Check warning on line 179 in src/test/java/de/tum/in/www1/artemis/lecture/AttachmentUnitsIntegrationTest.java

View check run for this annotation

Teamscale / teamscale-findings

src/test/java/de/tum/in/www1/artemis/lecture/AttachmentUnitsIntegrationTest.java#L179

Method `splitLectureFile_asInstructor_shouldRemoveSolutionSlides_and_removeBreakSlides` violates naming convention. Should be one of `[a-z][a-zA-Z0-9]*` https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=D1A876899A6DF40483B0139E7B0730D9
var filePart = createLectureFile(true);

LectureUnitInformationDTO lectureUnitSplitInfo = request.postWithMultipartFile("/api/lectures/" + lecture1.getId() + "/process-units", null, "process-units", filePart,
LectureUnitInformationDTO.class, HttpStatus.OK);
assertThat(lectureUnitSplitInfo.units()).hasSize(2);
assertThat(lectureUnitSplitInfo.numberOfPages()).isEqualTo(20);
lectureUnitSplitInfo = new LectureUnitInformationDTO(lectureUnitSplitInfo.units(), lectureUnitSplitInfo.numberOfPages(), true, true);

List<AttachmentUnit> attachmentUnits = List.of(request.postWithMultipartFile("/api/lectures/" + lecture1.getId() + "/attachment-units/split", lectureUnitSplitInfo,
"lectureUnitInformationDTO", filePart, AttachmentUnit[].class, HttpStatus.OK));
assertThat(attachmentUnits).hasSize(2);
assertThat(slideRepository.findAll()).hasSize(18); // 18 slides should be created for 2 attachment units (1 break slide is removed and 1 solution slide is removed)

List<Long> attachmentUnitIds = attachmentUnits.stream().map(AttachmentUnit::getId).toList();
List<AttachmentUnit> attachmentUnitList = attachmentUnitRepository.findAllById(attachmentUnitIds);

Check warning on line 194 in src/test/java/de/tum/in/www1/artemis/lecture/AttachmentUnitsIntegrationTest.java

View check run for this annotation

Teamscale / teamscale-findings

src/test/java/de/tum/in/www1/artemis/lecture/AttachmentUnitsIntegrationTest.java#L180-L194

Clone with 3 instances of length 10 https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=4A23D5B88955B75EA3150273DD76C250

assertThat(attachmentUnitList).hasSize(2);
assertThat(attachmentUnitList).isEqualTo(attachmentUnits);

// first unit
String attachmentPathFirstUnit = attachmentUnitList.get(0).getAttachment().getLink();
byte[] fileBytesFirst = request.get(attachmentPathFirstUnit, HttpStatus.OK, byte[].class);

try (PDDocument document = PDDocument.load(fileBytesFirst)) {
// 5 is the number of pages for the first unit (after break and solution are removed)
assertThat(document.getNumberOfPages()).isEqualTo(5);
document.close();
}

// second unit
String attachmentPathSecondUnit = attachmentUnitList.get(1).getAttachment().getLink();
byte[] fileBytesSecond = request.get(attachmentPathSecondUnit, HttpStatus.OK, byte[].class);

try (PDDocument document = PDDocument.load(fileBytesSecond)) {
// 13 is the number of pages for the second unit
assertThat(document.getNumberOfPages()).isEqualTo(13);
document.close();
}
}

private void testAllPreAuthorize() throws Exception {
request.postWithMultipartFile("/api/lectures/" + lecture1.getId() + "/process-units", null, "process-units", createLectureFile(true), LectureUnitInformationDTO.class,
HttpStatus.FORBIDDEN);
Expand All @@ -192,23 +235,39 @@
try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); PDDocument document = new PDDocument()) {
if (shouldBePDF) {
for (int i = 1; i <= 20; i++) {
document.addPage(new PDPage());
PDPageContentStream contentStream = new PDPageContentStream(document, document.getPage(i - 1));

if (i == 6 || i == 13) {
if (i == 6) {
contentStream.beginText();
contentStream.setFont(PDType1Font.TIMES_ROMAN, 12);
contentStream.newLineAtOffset(25, -15);
contentStream.showText("itp20..");
contentStream.newLineAtOffset(25, 500);
contentStream.showText("Break");
contentStream.newLineAtOffset(0, -15);
contentStream.showText("Have fun");
contentStream.close();
continue;
}

Check warning on line 252 in src/test/java/de/tum/in/www1/artemis/lecture/AttachmentUnitsIntegrationTest.java

View check run for this annotation

Teamscale / teamscale-findings

src/test/java/de/tum/in/www1/artemis/lecture/AttachmentUnitsIntegrationTest.java#L238-L252

Clone with 2 instances of length 13 https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=D875A33EAE8EB7D0879F06D566C5D73B

if (i == 7 || i == 14) {
if (i == 7) {
contentStream.beginText();
contentStream.setFont(PDType1Font.TIMES_ROMAN, 12);
contentStream.newLineAtOffset(25, -15);
contentStream.showText("itp20..");
contentStream.newLineAtOffset(25, 500);
contentStream.showText("Example solution");
contentStream.newLineAtOffset(0, -15);
contentStream.showText("First Unit");
contentStream.newLineAtOffset(0, -15);
contentStream.showText("Second Unit");
contentStream.endText();
contentStream.close();
continue;

Check warning on line 267 in src/test/java/de/tum/in/www1/artemis/lecture/AttachmentUnitsIntegrationTest.java

View check run for this annotation

Teamscale / teamscale-findings

src/test/java/de/tum/in/www1/artemis/lecture/AttachmentUnitsIntegrationTest.java#L255-L267

This method is a bit lengthy [0] and deeply nested in multiple places [1, 2, 3] which can make it hard to understand and maintain. Consider extracting helper methods or reducing the nesting by using early breaks or returns. [0] https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=58F014C2BE72AFC421B170242789C78A [1] https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=8C12B73771ADE5E7462437CE8A7CC72E [2] https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=8B275A51C6264D1544AEECE5158AFD28 [3] https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=feature%2Fremove-solution-slides%3AHEAD&id=596FAA0A2084B6C5F4DD8DF99E590B8E
}

if (i == 2 || i == 8) {
contentStream.beginText();
contentStream.setFont(PDType1Font.TIMES_ROMAN, 12);
contentStream.newLineAtOffset(25, -15);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,21 @@ describe('AttachmentUnitsComponent', () => {
expect(navigateSpy).toHaveBeenCalledOnce();
}));

it('should select process units checkbox', fakeAsync(() => {
it('should select remove break slides checkbox', fakeAsync(() => {
attachmentUnitsComponent.removeBreakSlides = false;
const onSelectRemoveBreakSlides = jest.spyOn(attachmentUnitsComponent, 'onSelectRemoveBreakSlides');
attachmentUnitsComponent.onSelectRemoveBreakSlides();
tick();
expect(onSelectRemoveBreakSlides).toHaveBeenCalledOnce();
expect(attachmentUnitsComponent.removeBreakSlides).toBeTrue();
}));

it('should select remove solution slides checkbox', fakeAsync(() => {
attachmentUnitsComponent.removeSolutionSlides = false;
const onSelectRemoveSolutionSlides = jest.spyOn(attachmentUnitsComponent, 'onSelectRemoveSolutionSlides');
attachmentUnitsComponent.onSelectRemoveSolutionSlides();
tick();
expect(onSelectRemoveSolutionSlides).toHaveBeenCalledOnce();
expect(attachmentUnitsComponent.removeSolutionSlides).toBeTrue();
}));
});
Loading