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

Communication: Only show accepted categories of accepted FAQs #9591

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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 @@ -31,10 +31,18 @@ public interface FaqRepository extends ArtemisJpaRepository<Faq, Long> {
""")
Set<String> findAllCategoriesByCourseId(@Param("courseId") Long courseId);

@Query("""
SELECT DISTINCT faq.categories
FROM Faq faq
WHERE faq.course.id = :courseId AND faq.faqState = :faqState
""")
Set<String> findAllCategoriesByCourseIdAndState(@Param("courseId") Long courseId, @Param("faqState") FaqState faqState);

Set<Faq> findAllByCourseIdAndFaqState(Long courseId, FaqState faqState);

@Transactional
@Modifying
void deleteAllByCourseId(Long courseId);

Faq findByIdAndFaqState(Long faqId, Faq faqState);
cremertim marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I am unsure what this line does, as it does not show up anywhere else. Also should the faqState be of type "Faq" or "FaqState"?

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 removed the unused code 👍

}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public ResponseEntity<FaqDTO> createFaq(@RequestBody Faq faq, @PathVariable Long
if (faq.getId() != null) {
throw new BadRequestAlertException("A new faq cannot already have an ID", ENTITY_NAME, "idExists");
}
checkPriviledgeForAcceptedElseThrow(faq, courseId);
checkRoleForCourse(faq.getFaqState(), courseId, Role.INSTRUCTOR);
cremertim marked this conversation as resolved.
Show resolved Hide resolved
if (faq.getCourse() == null || !faq.getCourse().getId().equals(courseId)) {
throw new BadRequestAlertException("Course ID in path and FAQ do not match", ENTITY_NAME, "courseIdMismatch");
}
Expand All @@ -105,9 +105,14 @@ public ResponseEntity<FaqDTO> updateFaq(@RequestBody Faq faq, @PathVariable Long
if (faqId == null || !faqId.equals(faq.getId())) {
throw new BadRequestAlertException("Id of FAQ and path must match", ENTITY_NAME, "idNull");
}
checkPriviledgeForAcceptedElseThrow(faq, courseId);
if (faq.getFaqState() == FaqState.ACCEPTED) {
checkRoleForCourse(faq.getFaqState(), courseId, Role.INSTRUCTOR);
}
Faq existingFaq = faqRepository.findByIdElseThrow(faqId);
checkPriviledgeForAcceptedElseThrow(existingFaq, courseId);
if (existingFaq.getFaqState() == FaqState.ACCEPTED) {
checkRoleForCourse(existingFaq.getFaqState(), courseId, Role.INSTRUCTOR);
}
checkRoleForCourse(existingFaq.getFaqState(), courseId, Role.INSTRUCTOR);
cremertim marked this conversation as resolved.
Show resolved Hide resolved
if (!Objects.equals(existingFaq.getCourse().getId(), courseId)) {
throw new BadRequestAlertException("Course ID of the FAQ provided courseID must match", ENTITY_NAME, "idNull");
}
Expand All @@ -116,19 +121,6 @@ public ResponseEntity<FaqDTO> updateFaq(@RequestBody Faq faq, @PathVariable Long
return ResponseEntity.ok().body(dto);
}

/**
* @param faq the faq to be checked *
* @param courseId the id of the course the faq belongs to
* @throws AccessForbiddenException if the user is not an instructor
*
*/
private void checkPriviledgeForAcceptedElseThrow(Faq faq, Long courseId) {
if (faq.getFaqState() == FaqState.ACCEPTED) {
Course course = courseRepository.findByIdElseThrow(courseId);
authCheckService.checkHasAtLeastRoleInCourseElseThrow(Role.INSTRUCTOR, course, null);
}
}

/**
* GET /courses/:courseId/faqs/:faqId : get the faq with the id faqId.
*
Expand All @@ -144,6 +136,7 @@ public ResponseEntity<FaqDTO> getFaq(@PathVariable Long faqId, @PathVariable Lon
if (faq.getCourse() == null || !faq.getCourse().getId().equals(courseId)) {
throw new BadRequestAlertException("Course ID in path and FAQ do not match", ENTITY_NAME, "courseIdMismatch");
}
checkShouldAccessNotAccepted(faq.getFaqState(), courseId);
FaqDTO dto = new FaqDTO(faq);
return ResponseEntity.ok(dto);
}
Expand Down Expand Up @@ -176,9 +169,10 @@ public ResponseEntity<Void> deleteFaq(@PathVariable Long faqId, @PathVariable Lo
*/
@GetMapping("courses/{courseId}/faqs")
@EnforceAtLeastStudentInCourse
public ResponseEntity<Set<FaqDTO>> getFaqForCourse(@PathVariable Long courseId) {
public ResponseEntity<Set<FaqDTO>> getFaqsForCourse(@PathVariable Long courseId) {
log.debug("REST request to get all Faqs for the course with id : {}", courseId);
Set<Faq> faqs = faqRepository.findAllByCourseId(courseId);
Set<Faq> faqs = authCheckService.isAtLeastTeachingAssistantInCourse(courseId) ? faqRepository.findAllByCourseId(courseId)
: faqRepository.findAllByCourseIdAndFaqState(courseId, FaqState.ACCEPTED);
cremertim marked this conversation as resolved.
Show resolved Hide resolved
cremertim marked this conversation as resolved.
Show resolved Hide resolved
Set<FaqDTO> faqDTOS = faqs.stream().map(FaqDTO::new).collect(Collectors.toSet());
return ResponseEntity.ok().body(faqDTOS);
}
Expand All @@ -194,6 +188,7 @@ public ResponseEntity<Set<FaqDTO>> getFaqForCourse(@PathVariable Long courseId)
@EnforceAtLeastStudentInCourse
public ResponseEntity<Set<FaqDTO>> getAllFaqsForCourseByStatus(@PathVariable Long courseId, @PathVariable FaqState faqState) {
log.debug("REST request to get all Faqs for the course with id : " + courseId + "and status " + faqState, courseId);
checkShouldAccessNotAccepted(faqState, courseId);
Set<Faq> faqs = faqRepository.findAllByCourseIdAndFaqState(courseId, faqState);
Set<FaqDTO> faqDTOS = faqs.stream().map(FaqDTO::new).collect(Collectors.toSet());
return ResponseEntity.ok().body(faqDTOS);
Expand All @@ -210,7 +205,34 @@ public ResponseEntity<Set<FaqDTO>> getAllFaqsForCourseByStatus(@PathVariable Lon
public ResponseEntity<Set<String>> getFaqCategoriesForCourse(@PathVariable Long courseId) {
log.debug("REST request to get all Faq Categories for the course with id : {}", courseId);
Set<String> faqs = faqRepository.findAllCategoriesByCourseId(courseId);
return ResponseEntity.ok().body(faqs);
}

@GetMapping("courses/{courseId}/faq-categories/{faqState}")
@EnforceAtLeastStudentInCourse
public ResponseEntity<Set<String>> getFaqCategoriesForCourseByState(@PathVariable Long courseId, @PathVariable FaqState faqState) {
log.debug("REST request to get all Faq Categories for the course with id : {} and the state {}", courseId, faqState);
checkShouldAccessNotAccepted(faqState, courseId);
Set<String> faqs = faqRepository.findAllCategoriesByCourseIdAndState(courseId, faqState);
return ResponseEntity.ok().body(faqs);
}

/**
* @param faqState the faqState to be checked *
* @param courseId the id of the course the faq belongs to
* @param role the required role of the user
* @throws AccessForbiddenException if the user is not atleast role
*
*/
private void checkRoleForCourse(FaqState faqState, Long courseId, Role role) {
Course course = courseRepository.findByIdElseThrow(courseId);
authCheckService.checkHasAtLeastRoleInCourseElseThrow(role, course, null);
}
cremertim marked this conversation as resolved.
Show resolved Hide resolved

private void checkShouldAccessNotAccepted(FaqState faqState, Long courseId) {
if (faqState != FaqState.ACCEPTED) {
checkRoleForCourse(faqState, courseId, Role.TEACHING_ASSISTANT);
}
}

}
6 changes: 6 additions & 0 deletions src/main/webapp/app/faq/faq.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,10 @@ export class FaqService {
const faqText = `${faq.questionTitle ?? ''} ${faq.questionAnswer ?? ''}`.toLowerCase();
return tokens.every((token) => faqText.includes(token));
}

findAllCategoriesByCourseIdAndCategory(courseId: number, faqState: FaqState) {
return this.http.get<string[]>(`${this.resourceUrl}/${courseId}/faq-categories/${faqState}`, {
observe: 'response',
});
}
}
16 changes: 15 additions & 1 deletion src/main/webapp/app/faq/faq.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,26 @@ import { AlertService } from 'app/core/util/alert.service';
import { Observable, catchError, map, of } from 'rxjs';
import { FaqService } from 'app/faq/faq.service';
import { FaqCategory } from 'app/entities/faq-category.model';
import { FaqState } from 'app/entities/faq.model';

export function loadCourseFaqCategories(courseId: number | undefined, alertService: AlertService, faqService: FaqService): Observable<FaqCategory[]> {
export function loadCourseFaqCategories(courseId: number | undefined, alertService: AlertService, faqService: FaqService, faqState?: FaqState): Observable<FaqCategory[]> {
if (courseId === undefined) {
return of([]);
}

if (faqState) {
return faqService.findAllCategoriesByCourseIdAndCategory(courseId, faqState).pipe(
map((categoryRes: HttpResponse<string[]>) => {
const existingCategories = faqService.convertFaqCategoriesAsStringFromServer(categoryRes.body || []);
return existingCategories;
}),
catchError((error: HttpErrorResponse) => {
onError(alertService, error);
return of([]);
}),
);
}
cremertim marked this conversation as resolved.
Show resolved Hide resolved

return faqService.findAllCategoriesByCourseId(courseId).pipe(
map((categoryRes: HttpResponse<string[]>) => {
const existingCategories = faqService.convertFaqCategoriesAsStringFromServer(categoryRes.body || []);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export class CourseFaqComponent implements OnInit, OnDestroy {

courseId: number;
faqs: Faq[];
faqState = FaqState.ACCEPTED;

filteredFaqs: Faq[];
existingCategories: FaqCategory[];
Expand Down Expand Up @@ -64,15 +65,15 @@ export class CourseFaqComponent implements OnInit, OnDestroy {
}

private loadCourseExerciseCategories(courseId: number) {
loadCourseFaqCategories(courseId, this.alertService, this.faqService).subscribe((existingCategories) => {
loadCourseFaqCategories(courseId, this.alertService, this.faqService, this.faqState).subscribe((existingCategories) => {
this.existingCategories = existingCategories;
this.hasCategories = existingCategories.length > 0;
});
}
cremertim marked this conversation as resolved.
Show resolved Hide resolved

private loadFaqs() {
this.faqService
.findAllByCourseIdAndState(this.courseId, FaqState.ACCEPTED)
.findAllByCourseIdAndState(this.courseId, this.faqState)
.pipe(map((res: HttpResponse<Faq[]>) => res.body))
.subscribe({
next: (res: Faq[]) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,24 @@ void deleteFaq_IdsDoNotMatch_shouldNotDeleteFAQ() throws Exception {

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void getFaq_shouldGetFaqByCourseId() throws Exception {
Set<Faq> faqs = faqRepository.findAllByCourseIdAndFaqState(this.course1.getId(), FaqState.PROPOSED);
void getFaq_InstructorsShouldGetAllFaqByCourseId() throws Exception {
Set<Faq> faqs = faqRepository.findAllByCourseId(this.course1.getId());
Set<FaqDTO> returnedFaqs = request.get("/api/courses/" + course1.getId() + "/faqs", HttpStatus.OK, Set.class);
assertThat(returnedFaqs).hasSize(faqs.size());
}
cremertim marked this conversation as resolved.
Show resolved Hide resolved

@Test
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
void getFaq_StudentsShouldOnlyGetAcceptedFaqByCourseId() throws Exception {
Set<Faq> faqs = faqRepository.findAllByCourseIdAndFaqState(this.course1.getId(), FaqState.ACCEPTED);
Set<FaqDTO> returnedFaqs = request.get("/api/courses/" + course1.getId() + "/faqs", HttpStatus.OK, Set.class);
assertThat(returnedFaqs).hasSize(faqs.size());
}
cremertim marked this conversation as resolved.
Show resolved Hide resolved

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void getFaq_ShouldGetFaqByCourseId() throws Exception {
Set<Faq> faqs = faqRepository.findAllByCourseId(this.course1.getId());
Set<FaqDTO> returnedFaqs = request.get("/api/courses/" + course1.getId() + "/faqs", HttpStatus.OK, Set.class);
cremertim marked this conversation as resolved.
Show resolved Hide resolved
assertThat(returnedFaqs).hasSize(faqs.size());
}
Expand All @@ -194,4 +210,21 @@ void getFaq_shouldGetFaqByCourseIdAndState() throws Exception {
assertThat(returnedFaqs).hasSize(faqs.size());
}

@Test
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
void getFaqs_StudentShouldOnlyGetAcceptedFaqByCourse() throws Exception {
Set<Faq> faqs = faqRepository.findAllByCourseIdAndFaqState(course1.getId(), FaqState.ACCEPTED);
Set<FaqDTO> returnedFaqs = request.get("/api/courses/" + course1.getId() + "/faqs", HttpStatus.OK, Set.class);
assertThat(returnedFaqs).hasSize(faqs.size());
assertThat(returnedFaqs.size()).isEqualTo(faqs.size());
}
cremertim marked this conversation as resolved.
Show resolved Hide resolved

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void getFaqs_InstructorShouldGetALLFaqByCourse() throws Exception {
Set<Faq> faqs = faqRepository.findAllByCourseId(course1.getId());
Set<FaqDTO> returnedFaqs = request.get("/api/courses/" + course1.getId() + "/faqs", HttpStatus.OK, Set.class);
assertThat(returnedFaqs).hasSize(faqs.size());
}
cremertim marked this conversation as resolved.
Show resolved Hide resolved

}
21 changes: 21 additions & 0 deletions src/test/javascript/spec/service/faq.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,27 @@ describe('Faq Service', () => {
expect(expectedResult.body).toEqual(expected);
});

it('should find all categories by courseId and faqState', () => {
const category = {
color: '#6ae8ac',
category: 'category1',
} as FaqCategory;
const returnedFromService = { categories: [JSON.stringify(category)] };
const expected = { ...returnedFromService };
const courseId = 1;
const faqState = FaqState.ACCEPTED;
service
.findAllCategoriesByCourseIdAndCategory(courseId, faqState)
cremertim marked this conversation as resolved.
Show resolved Hide resolved
.pipe(take(1))
.subscribe((resp) => (expectedResult = resp));
cremertim marked this conversation as resolved.
Show resolved Hide resolved
const req = httpMock.expectOne({
url: `api/courses/${courseId}/faq-categories/${faqState}`,
method: 'GET',
});
req.flush(returnedFromService);
expect(expectedResult.body).toEqual(expected);
});

it('should set add active filter correctly', () => {
let activeFilters = new Set<string>();
activeFilters = service.toggleFilter('category1', activeFilters);
Expand Down
Loading