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

General: Add capability to remove course icon #5691

Merged
merged 29 commits into from
Nov 12, 2022

Conversation

rriyaldhi
Copy link
Member

@rriyaldhi rriyaldhi commented Sep 28, 2022

Checklist

General

Client

  • I followed the coding and design guidelines and ensured that the layout is responsive.
  • 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 added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.

Motivation and Context

#5526

Description

When Delete Icon Button is clicked, the course icon is set to null. When the Save Button is clicked, the Course Update API will be called. The Course Update API will set the course icon to null and delete the previous course icon file.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Course with Icon
  1. Log in to Artemis
  2. Navigate to Course Administration
  3. Open Course Detail page
  4. Click Edit Button
  5. Click Delete Button under the Course Icon
  6. Click Save
  7. The Course Icon should be removed

Review Progress

Code Review

  • Review 1
  • Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Branch Line
course-update.component.ts 70.24% 87%

Screenshots

Desktop
Screen Shot 2022-10-19 at 2 45 12 PM

Mobile
Screen Shot 2022-10-19 at 2 45 28 PM

@rriyaldhi rriyaldhi requested a review from a team as a code owner September 28, 2022 23:25
@github-actions github-actions bot added client Pull requests that update TypeScript code. (Added Automatically!) cypress Pull requests that update cypress tests. (Added Automatically!) tests labels Sep 28, 2022
@artemis-bot artemis-bot added this to In progress in Artemis Development Sep 28, 2022
@rriyaldhi rriyaldhi marked this pull request as draft September 28, 2022 23:25
@rriyaldhi rriyaldhi changed the title General: Add capability to remove course icon General: Add capability to remove course icon Sep 28, 2022
@rriyaldhi rriyaldhi marked this pull request as ready for review October 5, 2022 19:48
@artemis-bot artemis-bot moved this from In progress to Ready for review in Artemis Development Oct 5, 2022
@b-fein b-fein linked an issue Oct 6, 2022 that may be closed by this pull request
Copy link
Contributor

@ge65cer ge65cer 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 TS1, works! My suggestions:

  • Remove the confirmation dialog, as clicking on "cancel" aborts the course editing and the icon is still there. You already need to confirm deleting by clicking on "save".
  • Move the button next to the upload form, maybe label it "Remove Icon". In my opinion it can also does not need to be red colored, as it is not a destructive action.

Screenshot 2022-10-07 at 21 46 12

Feel free to wait for feedback from other reviewers before applying these requests.

Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

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

Looked through the code and left some comments.
I mostly agree with Moritz‘ comments, but I actually like the red color of the button, since you’re still deleting something, even though you have to save the modification afterwards.

cy.get('.no-image').should('exist');
cy.intercept(PUT, BASE_API + 'courses').as('updateCourseQuery');
cy.get('#save-entity').click();
cy.wait('@updateCourseQuery').then((_) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can also leave out the _ completely when it’s unused

Artemis Development automation moved this from Ready for review to Review in progress Oct 12, 2022
Copy link

@JonasGeigerTUM JonasGeigerTUM left a comment

Choose a reason for hiding this comment

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

Deleting the course icon works as expected. Tested on ts1

Copy link

@LiaLism LiaLism left a comment

Choose a reason for hiding this comment

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

Removing icon worked.

@rriyaldhi rriyaldhi moved this from Review in progress to Reviewer approved in Artemis Development Nov 11, 2022
Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

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

Rechecked thee code changes, looks good to me

@krusche krusche added this to the 5.12.3 milestone Nov 12, 2022
@krusche krusche merged commit ed8b2a4 into develop Nov 12, 2022
@krusche krusche deleted the feature/course/delete-icon branch November 12, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) cypress Pull requests that update cypress tests. (Added Automatically!) ready to merge tests
Projects
No open projects
Artemis Development
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

Remove course icon
8 participants