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

Tutorial groups: Fix a usability issue when creating new tutorial for campus and language suggestions #7389

Merged

Conversation

max-bergmann
Copy link
Contributor

@max-bergmann max-bergmann commented Oct 16, 2023

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the coding and design guidelines.
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
  • I followed the coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

This PR is designed to improve usability when creating tutor groups. The PR offers the default selection of English and German. For the campus search, the default values have been extended. I.e. all possible values of the campus the instructor is involved in are now offered - not only the possibilities of the respective course

Description

fixes #7292

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 2 Courses where the user is instructor
  • 2 Tutor Groups
  1. Log in to Artemis
  2. Navigate to Course Administration
  3. Create one Tutorial --> check whether the default values for english and german are available.
  4. Switch to 2nd course --> create another Tutorial
  5. Check if the previouly inserted campus is visible.
  6. Repeat steps for editing of a tutorial

Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Line Coverage Confirmation (assert/expect)
create-tutorial-group.component.ts 98%

Screenshots

image
image

@max-bergmann max-bergmann requested a review from a team as a code owner October 16, 2023 12:26
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Oct 16, 2023
@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Oct 17, 2023
@@ -161,7 +162,7 @@ public ResponseEntity<Set<String>> getUniqueLanguageValues(@PathVariable Long co
public ResponseEntity<List<TutorialGroup>> getAllForCourse(@PathVariable Long courseId) {
log.debug("REST request to get all tutorial groups of course with id: {}", courseId);
var course = courseRepository.findByIdElseThrow(courseId);
var user = userRepository.getUserWithGroupsAndAuthorities();
var user = userRepository.getUser();
Copy link
Contributor

Choose a reason for hiding this comment

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

The groups are needed in the next call (checkHasAtLeastRoleInCourseElseThrow), which currently leads to an additional database request.

Copy link
Contributor

@sarpsahinalp sarpsahinalp left a comment

Choose a reason for hiding this comment

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

Tested on ts3 the changes seem to be working!
image

Copy link

@JanaNF JanaNF left a comment

Choose a reason for hiding this comment

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

Maunal tested on legacy ts2. Worked mostly as expected.

But for the languages: If I added a new language (e.g. Russisch) it did not show up as a default value for the other tutorial (like it did with the locations). I don't know if this is something you want to happen or not. Please let me know.

@max-bergmann
Copy link
Contributor Author

Maunal tested on legacy ts2. Worked mostly as expected.

But for the languages: If I added a new language (e.g. Russisch) it did not show up as a default value for the other tutorial (like it did with the locations). I don't know if this is something you want to happen or not. Please let me know.

Thanks for your review. Originally, the change was only meant for several campuses in different courses, but I have now included the change for the languages as well.

@max-bergmann max-bergmann requested a review from JanaNF October 29, 2023 16:45
Copy link

@vinceclifford vinceclifford left a comment

Choose a reason for hiding this comment

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

I reviewed the PR previously with Jana when the selection between German and English was still working.

I tested on TS3 and the requirements (the language selection) were not fulfilled.
Firstly, I can't choose between English and German. There is no drop-down menu. Screenshot of me entering German down below.
Secondly, previous inputs are not "transferred" to different courses. If I select Straubing as the campus in Course1 and then go to Course2, I will not get Straubing recommended. However, if I create another tutorial group in Course1 I will see my previous input. I don't know if this is as it should be.
However, the selection of German and English is not shown to me.

Screenshot 2023-10-29 at 18 28 23

JanaNF
JanaNF previously approved these changes Oct 29, 2023
Copy link

@JanaNF JanaNF left a comment

Choose a reason for hiding this comment

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

Tested again on ts2. Worked as expected

@milljoniaer milljoniaer temporarily deployed to artemis-test4.artemis.cit.tum.de October 30, 2023 15:05 — with GitHub Actions Inactive
milljoniaer
milljoniaer previously approved these changes Oct 30, 2023
Copy link
Contributor

@milljoniaer milljoniaer left a comment

Choose a reason for hiding this comment

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

Tested ts4, works as expected

Copy link
Contributor

@Strohgelaender Strohgelaender left a comment

Choose a reason for hiding this comment

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

Please implement my change request above, this currently leads to unneccesary database requests.

Copy link

@vinceclifford vinceclifford left a comment

Choose a reason for hiding this comment

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

Retested on TS3 during testing session. Works as expected

Copy link
Contributor

@rstief rstief left a comment

Choose a reason for hiding this comment

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

Tested on legacy ts3 in testing session.

Copy link
Collaborator

@MaximilianAnzinger MaximilianAnzinger left a comment

Choose a reason for hiding this comment

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

Tested during testing session

Copy link
Contributor

@lennart-keller lennart-keller left a comment

Choose a reason for hiding this comment

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

Tested in testing session

Copy link
Contributor

@florian-glombik florian-glombik left a comment

Choose a reason for hiding this comment

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

No issues found in testing session on ts3

Copy link
Contributor

@basak-akan basak-akan left a comment

Choose a reason for hiding this comment

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

Tested in testing session worked well

@krusche krusche changed the title Tutorial groups: Usability issue for creating new tutorial for campus and language suggestions Tutorial groups: Fix a usability issue for creating new tutorial for campus and language suggestions Nov 7, 2023
@krusche krusche added this to the 6.6.5 milestone Nov 7, 2023
@krusche krusche merged commit 8398aa3 into develop Nov 7, 2023
32 of 35 checks passed
@krusche krusche deleted the Tutorial/tutorial-usability-issues-in-the-creation-of-tutorial branch November 7, 2023 21:00
@krusche krusche changed the title Tutorial groups: Fix a usability issue for creating new tutorial for campus and language suggestions Tutorial groups: Fix a usability issue when creating new tutorial for campus and language suggestions Nov 7, 2023
b-fein pushed a commit that referenced this pull request Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix client Pull requests that update TypeScript code. (Added Automatically!) ready to merge server Pull requests that update Java code. (Added Automatically!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tutorial: Usability issues in the creation of tutorial. The search text fields does not work as expected.