Bug/cite 221 #276

PradnyaC11 · 2024-07-16T23:39:21Z

Guidelines for Pull Requests

If you haven't yet read our code review guidelines, please do so, You can find them here.

Please confirm the following by adding an x for each item (turn [ ] into [x]).

I have removed all code style changes that are not necessary (e.g. changing blanks across the whole file that don’t need to be changed, adding empty lines in parts other than your own code)
I am not making any changes to files that don’t have any effect (e.g. imports added that don’t need to be added)
I do not have any sysout statements in my code or commented out code that isn’t needed anymore
I am not reformatting any files in the wrong format or without cause.
I am not changing file encoding or line endings to something else than UTF-8, LF
My pull request does not show an insane amount of files being changed although my ticket only requires a few files being changed
I have added Javadoc/documentation where appropriate
I have added test cases where appropriate
I have explained any part of my code/implementation decisions that is not be self-explanatory

Please provide a brief description of your ticket

unique: Spring Data: Unique field in MongoDB document

... Put ticket description here and add link to ticket ...
https://diging.atlassian.net/browse/CITE-221

Are there any other pull requests that this one depends on?

diging/citesphere-model#51
diging/citesphere-model#52

Anything else the reviewer needs to know?

... describe here ...

…ction & group

diging-jenkins · 2024-07-16T23:39:25Z

Can one of the admins verify this patch?

jdamerow

I haven't run this but that does not look right. If there is a duplicate key, then the duplicates should be deleted, so that only one group/citation/collection is left. The user should not get an error, instead the issue should be resolved.

jdamerow · 2024-07-17T14:59:26Z

citesphere/src/main/java/edu/asu/diging/citesphere/core/service/impl/CitationManager.java

@@ -248,13 +250,15 @@ public ICitation updateCitationFromZotero(IUser user, String groupId, String ite
            return citation;
        } catch (HttpClientErrorException ex) {
            throw new CannotFindCitationException(ex);
+        } catch(DuplicateKeyException ex) {
+            throw ex;


no point in throwing an exception you catch; you can just let it be thrown

jdamerow · 2024-07-17T15:01:58Z

citesphere/src/main/java/edu/asu/diging/citesphere/core/service/impl/GroupManager.java

-            return (ICitationGroup) groupOptional.get();
+        if (groupOptional.isPresent()) {
+            ICitationGroup group = groupOptional.get();
+            if (!group.getUsers().contains(user.getUsername())) {


if the user does not have access to the group, then they shouldn't be added.

jdamerow

A new model version has been published but it doesn't look like pom.xml is being changed?

jdamerow · 2024-10-25T21:24:01Z

Resolve conflicts please

jdamerow · 2024-10-29T15:51:03Z

citesphere/pom.xml

@@ -25,7 +25,7 @@
        <spring.kafka.version>2.2.6.RELEASE</spring.kafka.version>
        <spring-social-zotero.version>0.12</spring-social-zotero.version>
        <citesphere.messages.version>0.4</citesphere.messages.version>
-        <citesphere.model.version>1.19</citesphere.model.version>
+        <citesphere.model.version>1.23-SNAPSHOT</citesphere.model.version>


this needs to be a proper version

jdamerow · 2024-10-29T15:58:21Z

citesphere/src/main/java/edu/asu/diging/citesphere/core/service/impl/CitationManager.java

@@ -379,14 +383,17 @@ public CitationResults getGroupItems(IUser user, String groupId, String collecti

        ICitationGroup group = null;
        Optional<ICitationGroup> groupOptional = groupRepository.findFirstByGroupId(new Long(groupId));
-        if (!groupOptional.isPresent() || !groupOptional.get().getUsers().contains(user.getUsername())) {
+        if (!groupOptional.isPresent()) {


hm, I'm fairly certain this create a security issue here. Basically, doesn't this say "if the group does not yet locally exist, get it from zotero" (that's fine zotero will complain if the user does not have access), however, "if the group does exist, let's just add the user to the group" (this seems wrong, where is the check if the user has access?).

jdamerow · 2024-10-29T15:58:35Z

citesphere/src/main/java/edu/asu/diging/citesphere/core/service/impl/CitationManager.java

@@ -399,13 +406,14 @@ public CitationResults getGroupItems(IUser user, String groupId, String collecti
            long previousVersion = group.getContentVersion();
            // first update the group info
            // if we are using a previously stored group, delete it
+            ICitationGroup zeteroGroup = null;


spelling mistake

jdamerow · 2024-10-29T19:48:48Z

citesphere/src/main/java/edu/asu/diging/citesphere/core/service/impl/CitationManager.java

@@ -387,6 +391,9 @@ public CitationResults getGroupItems(IUser user, String groupId, String collecti
            }
        } else {
            group = groupOptional.get();
+            if (!group.getUsers().contains(user.getUsername())){
+                group.getUsers().add(user.getUsername());
+            }


this should not be needed, should it? this else should only be called if the group is present and the user has access to it?

jdamerow · 2024-11-13T19:23:51Z

Make it so, Jenkins.

diging-jenkins · 2024-11-13T19:24:33Z

Build failed. Maybe you broke the test cases?

jdamerow · 2024-11-13T19:29:35Z

Make it so, Jenkins.

diging-jenkins · 2024-11-13T19:30:15Z

Build failed. Maybe you broke the test cases?

jdamerow · 2024-11-13T19:30:38Z

There are test failures

jdamerow · 2024-11-13T19:31:08Z

test_addCitationToReferences_nullReferences(edu.asu.diging.citesphere.core.service.impl.CitationManagerTest)  Time elapsed: 0.004 sec  <<< ERROR!
java.lang.NullPointerException

jdamerow · 2024-11-13T19:31:39Z

test_addCitationToReferences_citationFound(edu.asu.diging.citesphere.core.service.impl.CitationManagerTest)

diging-jenkins · 2024-12-03T19:05:48Z

Jenkins successfully deployed Citesphere to be reviewed!

…nto bug/CITE-221

PradnyaC11 added 7 commits July 3, 2024 16:40

[CITE-221] Added config for creating mongo DB index

1ae05d4

[CITE-221] Added config to create mongo index

4b4e181

[CITE-221] Added index config for key and groupId for citation, colle…

a8fbf93

…ction & group

[CITE-221] Added duplicate key exception to methods

c7a356b

[CITE-221] Added duplicate key exception for group

7121561

[CITE-221] Added duplicate key exception for group

fec500a

[CITE-221] Added few for Duplicate key exceptions and updated logic

2ac9b55

PradnyaC11 requested a review from jdamerow July 16, 2024 23:39

PradnyaC11 marked this pull request as draft July 16, 2024 23:41

jdamerow requested changes Jul 17, 2024

View reviewed changes

jdamerow closed this Jul 17, 2024

PradnyaC11 added 4 commits July 17, 2024 16:40

[CITE-221] Reverted duplicate key exceptions changes

73c3aaf

[CITE-221] Updated logic so as to not throw duplicate key exception

0b2624a

[CITE-221] Updated logic to avoid getting duplicate key exception

a87f135

[CITE-221] Addressing PR comment

12f6aa3

PradnyaC11 reopened this Jul 19, 2024

PradnyaC11 requested a review from jdamerow July 19, 2024 22:08

PradnyaC11 marked this pull request as ready for review July 19, 2024 22:08

jdamerow reviewed Sep 10, 2024

View reviewed changes

[CITE-221] Updated citesphere model version in pom.xml

42d1c8e

PradnyaC11 requested a review from jdamerow September 24, 2024 17:27

jdamerow closed this Oct 25, 2024

PradnyaC11 reopened this Oct 28, 2024

Merge branch 'develop' into bug/CITE-221

d7a4219

jdamerow requested changes Oct 29, 2024

View reviewed changes

jdamerow closed this Oct 29, 2024

[CITE-221] Addressed PR comments

a8a977c

PradnyaC11 reopened this Oct 29, 2024

jdamerow requested changes Oct 29, 2024

View reviewed changes

jdamerow closed this Oct 29, 2024

[CITE-221] Addressed PR comments

b039e95

PradnyaC11 reopened this Oct 31, 2024

PradnyaC11 requested a review from jdamerow October 31, 2024 18:34

[CITE-221] Resolved code factor dependency issue

928ea95

jdamerow closed this Nov 13, 2024

[CITE-221] Fixed test cases

1bea700

PradnyaC11 reopened this Dec 3, 2024

Merge branch 'develop' into bug/CITE-221

87abbe4

PradnyaC11 added 2 commits December 3, 2024 12:10

[CITE-221] Fixed code factor issues

ac2599b

Merge branch 'bug/CITE-221' of https://github.com/diging/citesphere i…

426cfbc

…nto bug/CITE-221

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/cite 221 #276

Bug/cite 221 #276

PradnyaC11 commented Jul 16, 2024 •

edited

Loading

diging-jenkins commented Jul 16, 2024

jdamerow left a comment

jdamerow Jul 17, 2024

jdamerow Jul 17, 2024

jdamerow left a comment

jdamerow commented Oct 25, 2024

jdamerow Oct 29, 2024

jdamerow Oct 29, 2024

jdamerow Oct 29, 2024

jdamerow Oct 29, 2024

jdamerow commented Nov 13, 2024

diging-jenkins commented Nov 13, 2024

jdamerow commented Nov 13, 2024

diging-jenkins commented Nov 13, 2024

jdamerow commented Nov 13, 2024

jdamerow commented Nov 13, 2024

jdamerow commented Nov 13, 2024

diging-jenkins commented Dec 3, 2024

Bug/cite 221 #276

Are you sure you want to change the base?

Bug/cite 221 #276

Conversation

PradnyaC11 commented Jul 16, 2024 • edited Loading

Guidelines for Pull Requests

Please provide a brief description of your ticket

Are there any other pull requests that this one depends on?

Anything else the reviewer needs to know?

diging-jenkins commented Jul 16, 2024

jdamerow left a comment

Choose a reason for hiding this comment

jdamerow Jul 17, 2024

Choose a reason for hiding this comment

jdamerow Jul 17, 2024

Choose a reason for hiding this comment

jdamerow left a comment

Choose a reason for hiding this comment

jdamerow commented Oct 25, 2024

jdamerow Oct 29, 2024

Choose a reason for hiding this comment

jdamerow Oct 29, 2024

Choose a reason for hiding this comment

jdamerow Oct 29, 2024

Choose a reason for hiding this comment

jdamerow Oct 29, 2024

Choose a reason for hiding this comment

jdamerow commented Nov 13, 2024

diging-jenkins commented Nov 13, 2024

jdamerow commented Nov 13, 2024

diging-jenkins commented Nov 13, 2024

jdamerow commented Nov 13, 2024

jdamerow commented Nov 13, 2024

jdamerow commented Nov 13, 2024

diging-jenkins commented Dec 3, 2024

PradnyaC11 commented Jul 16, 2024 •

edited

Loading