Skip to content

Conversation

erodde
Copy link
Contributor

@erodde erodde commented Jul 28, 2025

Link to jira.
This is only part of the needed changes in mycore listed in MCR-3441. Only changes regarding mycore-orcid2 module in this PR.

@erodde erodde requested a review from golsch August 14, 2025 12:48
@erodde erodde marked this pull request as ready for review August 14, 2025 12:48

private static final String ORCID = "0000-0001-2345-6789";
public class MCRORCIDUserTest extends MCRStoreTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Module mycore-orcid2 was migrated from JUnit 4 to JUnit 5 in main. When rebasing next time, you can use the following migrated version of your updated test file:

/*
 * This file is part of ***  M y C o R e  ***
 * See https://www.mycore.de/ for details.
 *
 * MyCoRe is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation, either version 3 of the License, or
 * (at your option) any later version.
 *
 * MyCoRe is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with MyCoRe.  If not, see <http://www.gnu.org/licenses/>.
 */

package org.mycore.orcid2.user;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mycore.orcid2.user.MCRORCIDUser.ATTR_ORCID_ID;
import static org.mycore.resource.MCRResourceHelper.getResourceUrl;

import java.io.IOException;
import java.net.URISyntaxException;
import java.text.MessageFormat;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.stream.Collectors;

import org.jdom2.Element;
import org.jdom2.JDOMException;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mycore.access.MCRAccessException;
import org.mycore.common.MCRConstants;
import org.mycore.common.MCRSessionMgr;
import org.mycore.common.MCRSystemUserInformation;
import org.mycore.datamodel.metadata.MCRMetadataManager;
import org.mycore.datamodel.metadata.MCRObject;
import org.mycore.datamodel.metadata.MCRObjectID;
import org.mycore.mods.MCRMODSWrapper;
import org.mycore.orcid2.client.MCRORCIDCredential;
import org.mycore.orcid2.exception.MCRORCIDException;
import org.mycore.orcid2.util.MCRIdentifier;
import org.mycore.test.MCRJPAExtension;
import org.mycore.test.MCRMetadataExtension;
import org.mycore.test.MyCoReTest;
import org.mycore.user2.MCRUser;
import org.mycore.user2.MCRUserAttribute;
import org.mycore.user2.MCRUserManager;

@MyCoReTest
@ExtendWith(MCRJPAExtension.class)
@ExtendWith(MCRMetadataExtension.class)
public class MCRORCIDUserTest {

    private static final String ORCID_1 = "0000-0001-2345-6789";

    private static final String ORCID_2 = "0000-0002-3456-7895";

    private static final String ORCID_3 = "0000-0003-4567-8985";

    private static final String ACCESS_TOKEN = "accessToken";

    @BeforeEach
    public void setUp() {
        MCRSessionMgr.getCurrentSession().setUserInformation(MCRSystemUserInformation.SUPER_USER);
    }

    @Test
    public void testStoreGetCredentials() {
        MCRUser user = new MCRUser("junit");
        MCRUserManager.createUser(user);
        MCRORCIDUser orcidUser = new MCRORCIDUser(user);
        assertEquals(0, orcidUser.getCredentials().size());
        final MCRORCIDCredential credential = new MCRORCIDCredential(ACCESS_TOKEN);
        orcidUser.addCredential(ORCID_1, credential);
        // id_orcid + orcid_credential_orcid
        assertEquals(2, user.getAttributes().size());
        assertNotNull(user.getUserAttribute("orcid_credential_" + ORCID_1));
        assertEquals(ORCID_1, user.getUserAttribute("id_orcid"));
        assertEquals(credential, orcidUser.getCredentialByORCID(ORCID_1));
    }

    @Test
    public void testRemoveAllCredentials() {
        MCRUser user = new MCRUser("junit");
        MCRUserManager.createUser(user);
        MCRORCIDUser orcidUser = new MCRORCIDUser(user);
        final MCRORCIDCredential credential = new MCRORCIDCredential(ACCESS_TOKEN);
        orcidUser.addCredential(ORCID_1, credential);
        user.setUserAttribute("test", "test");
        orcidUser.removeAllCredentials();
        // id_orcid + test
        assertEquals(2, user.getAttributes().size());
        assertEquals(ORCID_1, user.getUserAttribute("id_orcid"));
        assertEquals("test", user.getUserAttribute("test"));
    }

    @Test
    public void testGetORCIDs() throws IOException, JDOMException, MCRAccessException, URISyntaxException {
        MCRUser user = new MCRUser("junit");
        user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_1));
        user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_2));
        user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_3));
        MCRUserManager.createUser(user);
        MCRORCIDUser orcidUser = new MCRORCIDUser(user);
        orcidUser.setAccessImpl(new MCRORCIDAccessUserAttributeImpl());

        // Get ORCIDs with MCRORCIDAccessUserAttributeImpl implementation
        Set<String> orcids = orcidUser.getORCIDs();
        assertEquals(3, orcids.size());

        Set<String> expected = Set.of(ORCID_1, ORCID_2, ORCID_3);
        assertEquals(expected, orcids);

        orcidUser.setAccessImpl(new MCRORCIDAccessModspersonImpl());
        // Get ORCIDs with MCRORCIDAccessModspersonImpl implementation but no reference
        orcids = orcidUser.getORCIDs();
        assertEquals(0, orcids.size());

        user.setUserAttribute("id_modsperson", "junit_modsperson_00000001");
        MCRUserManager.updateUser(user);

        MCRObject obj1 = new MCRObject(getResourceUrl(buildFilePath("junit_modsperson_00000001.xml")).toURI());
        MCRMetadataManager.create(obj1);
        // Get ORCIDs with MCRORCIDAccessModspersonImpl implementation and reference
        orcids = orcidUser.getORCIDs();
        assertEquals(3, orcids.size());
        assertEquals(expected, orcids);
    }

    @Test
    public void testAddORCIDUserAttributeImpl() {
        MCRUser user = new MCRUser("junit");
        MCRUserManager.createUser(user);
        MCRORCIDUser orcidUser = new MCRORCIDUser(user);
        orcidUser.setAccessImpl(new MCRORCIDAccessUserAttributeImpl());

        orcidUser.addORCID(ORCID_1);
        orcidUser.addORCID(ORCID_2);
        orcidUser.addORCID(ORCID_3);

        MCRUser userUpdated = MCRUserManager.getUser("junit");

        Set<MCRUserAttribute> attributes = userUpdated.getAttributes();
        assertEquals(3, attributes.size());

        Set<MCRUserAttribute> expected = Set.of(new MCRUserAttribute("id_orcid", ORCID_1),
            new MCRUserAttribute("id_orcid", ORCID_2),
            new MCRUserAttribute("id_orcid", ORCID_3));
        assertEquals(expected, attributes);
    }

    @Test
    public void testAddORCIDModspersonImpl() throws MCRAccessException, URISyntaxException, IOException, JDOMException {
        MCRUser user = new MCRUser("junit");
        MCRUserManager.createUser(user);
        MCRORCIDUser orcidUser = new MCRORCIDUser(user);
        orcidUser.setAccessImpl(new MCRORCIDAccessModspersonImpl());
        orcidUser.addORCID(ORCID_1); // check for no error if no reference

        MCRObject modsperson = new MCRObject(getResourceUrl(buildFilePath("junit_modsperson_00000002.xml")).toURI());
        MCRMetadataManager.create(modsperson);

        user.setUserAttribute("id_modsperson", "junit_modsperson_00000002");
        MCRUserManager.updateUser(user);

        orcidUser.addORCID(ORCID_1);
        orcidUser.addORCID(ORCID_2);
        orcidUser.addORCID(ORCID_3);

        MCRUser userUpdated = MCRUserManager.getUser("junit");

        Set<MCRUserAttribute> attributes = userUpdated.getAttributes();
        assertEquals(1, attributes.size());
        assertEquals("id_modsperson", attributes.iterator().next().getName());
        assertEquals("junit_modsperson_00000002", attributes.iterator().next().getValue());

        MCRObject modspersonUpdated = MCRMetadataManager.retrieveMCRObject(MCRObjectID
            .getInstance("junit_modsperson_00000002"));
        assertNotNull(modspersonUpdated);

        MCRMODSWrapper wrapper = new MCRMODSWrapper(modspersonUpdated);
        Element personName = wrapper.getElement("mods:name[@type='personal']");
        List<Element> nameIdentifiers = personName.getChildren("nameIdentifier", MCRConstants.MODS_NAMESPACE);
        assertEquals(3, nameIdentifiers.size());
        nameIdentifiers.forEach(a -> assertEquals("orcid", a.getAttributeValue("type")));

        Set<String> actualValues = nameIdentifiers.stream()
            .map(Element::getText)
            .collect(Collectors.toSet());
        Set<String> expectedValues = Set.of(
            ORCID_1,
            ORCID_2,
            ORCID_3);
        assertEquals(expectedValues, actualValues);

    }

    @Test
    void testAddORCIDInvalid() {
        assertThrows(
            MCRORCIDException.class,
            () -> {
                MCRUser user = new MCRUser("junit");
                MCRUserManager.createUser(user);
                MCRORCIDUser orcidUser = new MCRORCIDUser(user);
                orcidUser.setAccessImpl(new MCRORCIDAccessUserAttributeImpl());

                orcidUser.addORCID("abcd");
            });
    }

    @Test
    public void testGetIdentifiers() throws MCRAccessException, URISyntaxException, IOException, JDOMException {
        MCRUser user = new MCRUser("junit");
        user.getAttributes().add(new MCRUserAttribute("id_scopus", "87654321"));
        user.getAttributes().add(new MCRUserAttribute("id_something", "abcd"));
        user.getAttributes().add(new MCRUserAttribute("just_something", "1234"));

        MCRUserManager.createUser(user);
        MCRORCIDUser orcidUser = new MCRORCIDUser(user);
        orcidUser.setAccessImpl(new MCRORCIDAccessUserAttributeImpl());

        orcidUser.addORCID(ORCID_1);

        // getIdentifiers with MCRORCIDAccessUserAttributeImpl implementation
        Set<MCRIdentifier> identifiers = orcidUser.getIdentifiers();
        assertEquals(3, identifiers.size());
        Set<MCRIdentifier> expected = new HashSet<>(Set.of(new MCRIdentifier("scopus", "87654321"),
            new MCRIdentifier("something", "abcd"),
            new MCRIdentifier("orcid", "0000-0001-2345-6789")));
        assertEquals(expected, identifiers);

        // getIdentifiers with MCRORCIDAccessModspersonImpl implementation but no reference
        orcidUser.setAccessImpl(new MCRORCIDAccessModspersonImpl());
        identifiers = orcidUser.getIdentifiers();
        assertEquals(0, identifiers.size());

        user.setUserAttribute("id_modsperson", "junit_modsperson_00000001");
        MCRUserManager.updateUser(user);

        MCRObject obj1 = new MCRObject(getResourceUrl(buildFilePath("junit_modsperson_00000001.xml")).toURI());
        MCRMetadataManager.create(obj1);

        // getIdentifiers with MCRORCIDAccessModspersonImpl implementation and reference
        identifiers = orcidUser.getIdentifiers();
        assertEquals(5, identifiers.size());
        expected.add(new MCRIdentifier("orcid", ORCID_2));
        expected.add(new MCRIdentifier("orcid", ORCID_3));
        assertEquals(expected, identifiers);
    }

    private String buildFilePath(String fileName) {
        return new MessageFormat("/{0}/{1}", Locale.ROOT).format(
            new Object[] { this.getClass().getSimpleName(), fileName });
    }

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, you are targeting 2024.06.x, ... The above comment will be relevant for whoever is merging this into main at some point.

@toKrause toKrause requested review from toKrause and removed request for toKrause August 26, 2025 12:26
/**
* Used to encapsulate access to a user's ORCIDs.
*/
public interface MCRORCIDAccess {
Copy link
Member

Choose a reason for hiding this comment

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

I think the name isn't suitable. How about: MCRORCIDIDAttributeHandler?

if (modsperson != null) {
MCRMODSWrapper wrapper = new MCRMODSWrapper(modsperson);

Element personName = wrapper.getElement("mods:name[@type='personal']");
Copy link
Member

Choose a reason for hiding this comment

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

Refactor mods:name[@type='personal'] XPath into constant.

Comment on lines +41 to +43
MCRMODSWrapper wrapper = new MCRMODSWrapper(modsperson);
Element personName = wrapper.getElement("mods:name[@type='personal']");
return personName.getChildren("nameIdentifier", MCRConstants.MODS_NAMESPACE)
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: add helper method to get attributes. Also, can then be used in getIdentifiers.

Comment on lines +104 to +106
public void setAccessImpl(MCRORCIDAccess accessImpl) {
this.accessImpl = accessImpl;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's very nice. Either the implementation should be loaded via a constructor, which can also be protected, provided that the implementation is not loaded statically. Otherwise, the implementation should be loaded via the property for the tests.
Another way would be a getAccessImp method that you could control it via a mock.

Comment on lines +80 to +82
private MCRORCIDAccess accessImpl = MCRConfiguration2.getInstanceOfOrThrow(
MCRORCIDAccess.class, MCRORCIDConstants.CONFIG_PREFIX + "Access.Class");

Copy link
Member

Choose a reason for hiding this comment

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

Theoretically, the implementation could be loaded statically or as a single instance in the default constructor.

*/
public interface MCRORCIDAccess {

void addORCID(String orcid, MCRUser user) throws MCRAccessException;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this exception is appropriate.

user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_1));
user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_2));
user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_3));
MCRUserManager.createUser(user);
Copy link
Member

@golsch golsch Aug 27, 2025

Choose a reason for hiding this comment

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

Does MCRUserManager action really need to be done? Mock should be enough.

Comment on lines +116 to +129
orcidUser.setAccessImpl(new MCRORCIDAccessModspersonImpl());
// Get ORCIDs with MCRORCIDAccessModspersonImpl implementation but no reference
orcids = orcidUser.getORCIDs();
assertEquals(0, orcids.size());

user.setUserAttribute("id_modsperson", "junit_modsperson_00000001");
MCRUserManager.updateUser(user);

MCRObject obj1 = new MCRObject(getResourceAsURL("junit_modsperson_00000001.xml").toURI());
MCRMetadataManager.create(obj1);
// Get ORCIDs with MCRORCIDAccessModspersonImpl implementation and reference
orcids = orcidUser.getORCIDs();
assertEquals(3, orcids.size());
assertEquals(expected, orcids);
Copy link
Member

@golsch golsch Aug 27, 2025

Choose a reason for hiding this comment

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

General suggestion: create atomic tests for individual cases.

Comment on lines +102 to +104
user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_1));
user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_2));
user.getAttributes().add(new MCRUserAttribute(ATTR_ORCID_ID, ORCID_3));
Copy link
Member

Choose a reason for hiding this comment

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

You could mock MCRUser and hijack the getAttributes method.


private static final String ORCID = "0000-0001-2345-6789";
public class MCRORCIDUserTest extends MCRStoreTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Tests are always beneficial, of course. I believe there are various places where you can effectively mock MCRUser in order to test it in isolation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants