Merge branch 'develop' into chore/development/remove-deprecated-code

docs/dev/playwright.rst

@@ -96,6 +96,7 @@ Playwright tests rely on the Playwright Node.js library, browser binaries, and s
 
         npx playwright test src/test/playwright/tests/CourseManagement.spec.ts -g "Course creation"
 
+
 Test parallelization
 --------------------
 
@@ -113,3 +114,98 @@ file.
 
 To run tests sequentially (one after another), set the ``workers`` option to ``1``. To run tests within each file
 sequentially, while running test files in parallel, set the ``fullyParallel`` option to ``false``.
+
+
+Best practices for writing tests in Playwright
+----------------------------------------------
+
+1. **Use page objects for common interactions**:
+
+    Page objects are a design pattern that helps to abstract the details of the page structure and interactions. They
+    encapsulate the page elements and their interactions with the page. This makes the tests more readable and
+    maintainable.
+    Page objects are stored in the ``support/pageobjects`` folder. Each page object is implemented as a class containing
+    a Playwright page instance and may have instances of other page objects as well. Page object classes provide methods
+    performing common user actions or returning frequently used locators.
+    Page objects are registered as fixtures to make them easily accessible in tests without caring about their
+    initialization and teardown.
+
+2. **Use fixtures**:
+
+    Test fixture in Playwright is a setup environment that prepares the necessary conditions and state required for your
+    tests to run. It helps manage the initialization and cleanup tasks so that each test starts with a known state.
+    We use fixtures for all POMs and common test commands such as ``login``. Fixtures are defined in
+    ``support/fixtures.ts``.
+
+    To create a fixture, define its instance inside a corresponding existing type or define a new one:
+
+    .. code-block:: typescript
+
+        export type ArtemisPageObjects = {
+            loginPage: LoginPage;
+        }
+
+    2. Ensure the base test (``base``) extends the fixture type. Define a fixture with the relevant name and return the
+    desired instance as an argument of ``use()`` function as below:
+
+    .. code-block:: typescript
+
+        export const test = base.extend<ArtemisPageObjects>({
+            loginPage: async ({ page }) => new LoginPage(page)
+        });
+
+    3. Inject the fixture to tests when needed as an argument to the ``test()`` function as follows:
+
+    .. code-block:: typescript
+
+        test('Test name', async ({ fixtureName }) => {
+            // Test code
+        });
+
+3. **Use uniquely identifiable locators**:
+
+    Use unique locators to identify elements on the page. Playwright throws an error when interacting with a locator
+    that matches multiple elements on the page. To ensure uniqueness, use locators based on the element's
+    ``data-testid``, ``id``, unique ``class`` or a combination of these attributes.
+
+    Avoid using the ``nth()`` method or the ``nth-child`` selector, as they rely on the element’s position in the DOM
+    hierarchy. Use these methods only when iterating over multiple similar elements.
+
+    Avoid using locators that are prone to change. If a component lacks a unique selector,
+    add a ``data-testid`` attribute with a unique value to its template. This ensures that the component is easily
+    identifiable, making tests less likely to break when there are changes to the component.
+
+4. **Consider actionability of elements**
+
+    Checking for the state of an element before interacting with it is crucial to avoid flaky behavior. Actions like
+    clicking a button or typing into an input field require a particular state from the element, such as visible and
+    enabled, which makes it actionable.  Playwright ensures that the elements you interact with are actionable before
+    performing such actions.
+
+    However, some complex interactions may require additional checks to ensure the element is in the desired state. For
+    example, consider a case where we want to access the inner text of an element that is not visible yet. Use ``waitFor()``
+    function of a locator to wait for its ``visible`` state before accessing its inner text:
+
+    .. code-block:: typescript
+
+        await page.locator('.clone-url').waitFor({ state: 'visible' });
+        const urlText = await this.page.locator('.clone-url').innerText();
+
+    .. warning ::
+
+        Avoid using ``page.waitForSelector()`` function to wait for an element to appear on the page. This function
+        waits for the visibility in the DOM, but it does not guarantee that the element is actionable. Always
+        prefer the ``waitFor()`` function of a locator instead.
+
+    In some cases, we may need to wait for the page to load completely before interacting with its elements. Use
+    ``waitForLoadState()`` function to wait for the page to reach a specified load state:
+
+    .. code-block:: typescript
+
+        await page.waitForLoadState('load');
+
+    .. warning ::
+
+        Waiting for the page load state is not recommended if we are only interested in specific elements appearing on
+        the page - use ``waitFor()`` function of a locator instead.
+

src/main/java/de/tum/in/www1/artemis/repository/science/ScienceEventRepository.java

@@ -6,7 +6,11 @@
 
 import org.springframework.context.annotation.Profile;
 import org.springframework.data.jpa.repository.JpaRepository;
+import org.springframework.data.jpa.repository.Modifying;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.query.Param;
 import org.springframework.stereotype.Repository;
+import org.springframework.transaction.annotation.Transactional;
 
 import de.tum.in.www1.artemis.domain.science.ScienceEvent;
 import de.tum.in.www1.artemis.domain.science.ScienceEventType;
@@ -19,4 +23,13 @@
 public interface ScienceEventRepository extends JpaRepository<ScienceEvent, Long> {
 
     Set<ScienceEvent> findAllByType(ScienceEventType type);
+
+    @Transactional // ok because of modifying query
+    @Modifying
+    @Query("""
+            UPDATE ScienceEvent se
+            SET se.identity = :newIdentity
+            WHERE se.identity = :oldIdentity
+            """)
+    void renameIdentity(@Param("oldIdentity") String oldIdentity, @Param("newIdentity") String newIdentity);
 }

src/main/java/de/tum/in/www1/artemis/service/user/UserService.java

@@ -11,6 +11,7 @@
 import static de.tum.in.www1.artemis.domain.Authority.ADMIN_AUTHORITY;
 import static de.tum.in.www1.artemis.security.Role.ADMIN;
 import static de.tum.in.www1.artemis.security.Role.STUDENT;
+import static org.apache.commons.lang3.StringUtils.lowerCase;
 
 import java.net.URI;
 import java.time.Instant;
@@ -19,6 +20,7 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Supplier;
@@ -45,6 +47,7 @@
 import de.tum.in.www1.artemis.repository.AuthorityRepository;
 import de.tum.in.www1.artemis.repository.GuidedTourSettingsRepository;
 import de.tum.in.www1.artemis.repository.UserRepository;
+import de.tum.in.www1.artemis.repository.science.ScienceEventRepository;
 import de.tum.in.www1.artemis.security.SecurityUtils;
 import de.tum.in.www1.artemis.service.FilePathService;
 import de.tum.in.www1.artemis.service.FileService;
@@ -104,10 +107,12 @@ public class UserService {
 
     private final FileService fileService;
 
+    private final ScienceEventRepository scienceEventRepository;
+
     public UserService(UserCreationService userCreationService, UserRepository userRepository, AuthorityService authorityService, AuthorityRepository authorityRepository,
             CacheManager cacheManager, Optional<LdapUserService> ldapUserService, GuidedTourSettingsRepository guidedTourSettingsRepository, PasswordService passwordService,
             Optional<VcsUserManagementService> optionalVcsUserManagementService, Optional<CIUserManagementService> optionalCIUserManagementService,
-            InstanceMessageSendService instanceMessageSendService, FileService fileService) {
+            InstanceMessageSendService instanceMessageSendService, FileService fileService, ScienceEventRepository scienceEventRepository) {
         this.userCreationService = userCreationService;
         this.userRepository = userRepository;
         this.authorityService = authorityService;
@@ -120,6 +125,7 @@ public UserService(UserCreationService userCreationService, UserRepository userR
         this.optionalCIUserManagementService = optionalCIUserManagementService;
         this.instanceMessageSendService = instanceMessageSendService;
         this.fileService = fileService;
+        this.scienceEventRepository = scienceEventRepository;
     }
 
     /**
@@ -467,10 +473,11 @@ protected void anonymizeUser(User user) {
         final Set<String> originalGroups = user.getGroups();
         final String randomPassword = RandomUtil.generatePassword();
         final String userImageString = user.getImageUrl();
+        final String anonymizedLogin = lowerCase(RandomUtil.generateRandomAlphanumericString(), Locale.ENGLISH);
 
         user.setFirstName(USER_FIRST_NAME_AFTER_SOFT_DELETE);
         user.setLastName(USER_LAST_NAME_AFTER_SOFT_DELETE);
-        user.setLogin(RandomUtil.generateRandomAlphanumericString());
+        user.setLogin(anonymizedLogin);
         user.setPassword(randomPassword);
         user.setEmail(RandomUtil.generateRandomAlphanumericString() + USER_EMAIL_DOMAIN_AFTER_SOFT_DELETE);
         user.setRegistrationNumber(null);
@@ -482,6 +489,8 @@ protected void anonymizeUser(User user) {
         clearUserCaches(user);
         userRepository.flush();
 
+        scienceEventRepository.renameIdentity(originalLogin, anonymizedLogin);
+
         if (userImageString != null) {
             fileService.schedulePathForDeletion(FilePathService.actualPathForPublicPath(URI.create(userImageString)), 0);
         }

src/main/resources/config/liquibase/changelog/20240531210000_changelog.xml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog https://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-latest.xsd">
+    <changeSet id="20240531210000" author="rstief">
+        <dropForeignKeyConstraint baseTableName="competency" constraintName="fk_competency_linked_standardized_competency"/>
+        <addForeignKeyConstraint baseColumnNames="linked_standardized_competency_id" baseTableName="competency" constraintName="fk_competency_linked_standardized_competency" deferrable="false" initiallyDeferred="false" onDelete="SET NULL" onUpdate="RESTRICT" referencedColumnNames="id" referencedTableName="standardized_competency" validate="true"/>
+    </changeSet>
+</databaseChangeLog>

src/main/resources/config/liquibase/master.xml

@@ -11,6 +11,7 @@
     <include file="classpath:config/liquibase/changelog/20240418204415_changelog.xml" relativeToChangelogFile="false"/>
     <include file="classpath:config/liquibase/changelog/20240515204415_changelog.xml" relativeToChangelogFile="false"/>
     <include file="classpath:config/liquibase/changelog/20240522114700_changelog.xml" relativeToChangelogFile="false"/>
+    <include file="classpath:config/liquibase/changelog/20240531210000_changelog.xml" relativeToChangelogFile="false"/>
     <!-- NOTE: please use the format "YYYYMMDDhhmmss_changelog.xml", i.e. year month day hour minutes seconds and not something else! -->
     <!-- we should also stay in a chronological order! -->
     <!-- you can use the command 'date '+%Y%m%d%H%M%S'' to get the current date and time in the correct format -->

src/test/java/de/tum/in/www1/artemis/competency/StandardizedCompetencyIntegrationTest.java

@@ -18,10 +18,12 @@
 import org.springframework.security.test.context.support.WithMockUser;
 
 import de.tum.in.www1.artemis.AbstractSpringIntegrationIndependentTest;
+import de.tum.in.www1.artemis.domain.competency.Competency;
 import de.tum.in.www1.artemis.domain.competency.CompetencyTaxonomy;
 import de.tum.in.www1.artemis.domain.competency.KnowledgeArea;
 import de.tum.in.www1.artemis.domain.competency.Source;
 import de.tum.in.www1.artemis.domain.competency.StandardizedCompetency;
+import de.tum.in.www1.artemis.repository.CompetencyRepository;
 import de.tum.in.www1.artemis.repository.SourceRepository;
 import de.tum.in.www1.artemis.repository.competency.KnowledgeAreaRepository;
 import de.tum.in.www1.artemis.repository.competency.StandardizedCompetencyRepository;
@@ -60,6 +62,9 @@ class StandardizedCompetencyIntegrationTest extends AbstractSpringIntegrationInd
     @Autowired
     private StandardizedCompetencyUtilService standardizedCompetencyUtilService;
 
+    @Autowired
+    private CompetencyRepository competencyRepository;
+
     @BeforeEach
     void setupTestScenario() {
         userUtilService.addUsers(TEST_PREFIX, 1, 1, 1, 1);
@@ -197,6 +202,22 @@ void shouldDeleteCompetency() throws Exception {
                 assertThat(exists).isFalse();
             }
 
+            @Test
+            @WithMockUser(username = "admin", roles = "ADMIN")
+            void shouldDeleteCompetencyWithLinkedCompetency() throws Exception {
+                long deletedId = standardizedCompetency.getId();
+                Competency competency = new Competency("title", "description", null, null, null, false);
+                competency.setLinkedStandardizedCompetency(standardizedCompetency);
+                competency = competencyRepository.save(competency);
+
+                request.delete("/api/admin/standardized-competencies/" + deletedId, HttpStatus.OK);
+
+                boolean standardizedExists = standardizedCompetencyRepository.existsById(deletedId);
+                assertThat(standardizedExists).isFalse();
+                boolean competencyExists = competencyRepository.existsById(competency.getId());
+                assertThat(competencyExists).isTrue();
+            }
+
             @Test
             @WithMockUser(username = "admin", roles = "ADMIN")
             void shouldReturn404() throws Exception {

src/test/java/de/tum/in/www1/artemis/user/UserTestService.java

@@ -26,11 +26,14 @@
 import de.tum.in.www1.artemis.domain.Authority;
 import de.tum.in.www1.artemis.domain.Course;
 import de.tum.in.www1.artemis.domain.User;
+import de.tum.in.www1.artemis.domain.science.ScienceEvent;
+import de.tum.in.www1.artemis.domain.science.ScienceEventType;
 import de.tum.in.www1.artemis.exercise.programming.MockDelegate;
 import de.tum.in.www1.artemis.exercise.programming.ProgrammingExerciseUtilService;
 import de.tum.in.www1.artemis.repository.AuthorityRepository;
 import de.tum.in.www1.artemis.repository.CourseRepository;
 import de.tum.in.www1.artemis.repository.UserRepository;
+import de.tum.in.www1.artemis.repository.science.ScienceEventRepository;
 import de.tum.in.www1.artemis.security.Role;
 import de.tum.in.www1.artemis.service.connectors.ci.CIUserManagementService;
 import de.tum.in.www1.artemis.service.connectors.lti.LtiService;
@@ -82,12 +85,17 @@ public class UserTestService {
     @Autowired
     private ProgrammingExerciseUtilService programmingExerciseUtilService;
 
+    @Autowired
+    private ScienceEventRepository scienceEventRepository;
+
     private String TEST_PREFIX;
 
     private MockDelegate mockDelegate;
 
     public User student;
 
+    private ScienceEvent scienceEvent;
+
     private static final int numberOfStudents = 2;
 
     private static final int numberOfTutors = 1;
@@ -107,6 +115,12 @@ public void setup(String testPrefix, MockDelegate mockDelegate) throws Exception
         student = userRepository.findOneWithGroupsAndAuthoritiesByLogin(student.getLogin()).orElseThrow();
 
         users.forEach(user -> cacheManager.getCache(UserRepository.USERS_CACHE).evict(user.getLogin()));
+
+        final var event = new ScienceEvent();
+        event.setIdentity(student.getLogin());
+        event.setTimestamp(ZonedDateTime.now());
+        event.setType(ScienceEventType.EXERCISE__OPEN);
+        scienceEvent = scienceEventRepository.save(event);
     }
 
     public void tearDown() throws IOException {
@@ -127,6 +141,12 @@ private void assertThatUserWasSoftDeleted(User originalUser, User deletedUser) t
         assertThat(deletedUser.getRegistrationNumber()).isNull();
         assertThat(deletedUser.getImageUrl()).isNull();
         assertThat(deletedUser.getActivated()).isFalse();
+
+        // check only if owner of event is asserted
+        if (originalUser.getLogin().equals(scienceEvent.getIdentity())) {
+            final var deletedEvent = scienceEventRepository.findById(scienceEvent.getId()).orElseThrow();
+            assertThat(deletedEvent.getIdentity()).isEqualTo(deletedUser.getLogin());
+        }
     }
 
     private void assertThatUserWasNotSoftDeleted(User originalUser, User deletedUser) throws Exception {
@@ -138,6 +158,12 @@ private void assertThatUserWasNotSoftDeleted(User originalUser, User deletedUser
         assertThat(deletedUser.getEmail()).isEqualTo(originalUser.getEmail());
         assertThat(deletedUser.getRegistrationNumber()).isEqualTo(originalUser.getVisibleRegistrationNumber());
         assertThat(deletedUser.getImageUrl()).isEqualTo(originalUser.getImageUrl());
+
+        // check only if owner of event is asserted
+        if (originalUser.getLogin().equals(scienceEvent.getIdentity())) {
+            final var unchangedEvent = scienceEventRepository.findById(scienceEvent.getId()).orElseThrow();
+            assertThat(unchangedEvent.getIdentity()).isEqualTo(originalUser.getLogin());
+        }
     }
 
     // Test