story/VSPC-242 #345

jophals · 2024-06-07T00:24:31Z

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

(you can copy the ticket if it hasn't changed)

Users should be able to include external links on their slides, which will be visible on the public page.

Anything else the reviewer needs to know?

This is my first Java PR!

jophals · 2024-06-13T00:59:44Z

I reopened the PR and thought that the query was working. It's actually breaking the code in the current state that I've pushed. Can you help me with the JPA named queries? The error seems to be "No property findbySlide found for type ExternalLinkSlide"

jophals · 2024-06-13T01:05:51Z

Nevermind! I forgot to capitalize the "B", whoops

jdamerow

there are a bunch of files showing up as changed that actually have not been changed (just unnecessary new line changes, imports, etc)
there are still link display classes that are not needed and some unnecessary inheritance

jdamerow · 2024-06-13T17:25:35Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/ILinkSlide.java

@@ -0,0 +1,12 @@
+package edu.asu.diging.vspace.core.model;
+
+public interface ILinkSlide<T extends IVSpaceElement> extends IVSpaceElement {


I don't think we'll introduce anymore links at this point, so this seems overly complicated. Just and external link class for slides is enough

jdamerow · 2024-06-13T17:25:55Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/ISlide.java

+
+    IVSImage getImage();
+
+    void setImage(IVSImage image);


what are these images for?

i believe that these are for the ImageBlocks

but why would that be needed here?

jdamerow · 2024-06-13T17:26:08Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/display/ISlideExternalLinkDisplay.java

+
+import edu.asu.diging.vspace.core.model.IExternalLinkSlide;
+
+public interface ISlideExternalLinkDisplay extends ILinkDisplay {


jdamerow · 2024-06-13T17:26:16Z

...ce/src/main/java/edu/asu/diging/vspace/core/model/display/impl/SlideExternalLinkDisplay.java

+import edu.asu.diging.vspace.core.model.impl.ExternalLinkSlide;
+
+@Entity
+public class SlideExternalLinkDisplay extends LinkDisplay implements ISlideExternalLinkDisplay {


jdamerow · 2024-06-13T17:27:14Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/ExternalLinkSlide.java

+    }
+
+    @Override
+    public ExternalLinkValue getTarget() {


not needed. external links have a string field with the url they opint to. that's it.

jdamerow · 2024-06-13T17:34:33Z

vspace/src/main/webapp/WEB-INF/views/exhibition/space.html

@@ -486,6 +491,7 @@
                    linkDisplay.css('transform', 'rotate('+link.rotation+'deg)');
                    linkDisplay.css('fill', 'grey'); 
                    linkDisplay.css('color', 'rgba(128,128,128,1)');
+                    console.log('LINK DISPLAY: ', linkDisplay);


jdamerow · 2024-06-13T17:34:39Z

vspace/src/main/webapp/WEB-INF/views/exhibition/space.html

@@ -463,7 +467,8 @@
            })
            externalLinks.forEach(function(link,index){
                if(link!=null){
-                    var posX = parseInt($("#space").css('margin-left')) + $("#space").position().left; 
+                    var posX = parseInt($("#space").css('margin-left')) + $("#space").position().left;
+                    console.log('posX: ', parseInt($("#space").css('margin-left')), $("#space").position());


jdamerow · 2024-06-13T17:35:00Z

vspace/src/main/webapp/WEB-INF/views/exhibition/space.html

it doesn't seem necessary for this file to change?

jdamerow · 2024-06-13T17:36:36Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+
+			.imgDiv {
+				display: inline-block;
+			}


are all of these css changes needed?

jdamerow · 2024-06-13T17:37:00Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+           				$(imageblock[0]).mouseenter(onMouseEnter).mouseleave(onMouseLeave).dblclick(onDoubleClick);
+           			}
+           			// Reset data-attribute to get current selected image ID
+           			$("#uploadImage").data('value', '');


there shouldn't be any image changes needed for this ticket

slide.html errors, and deleted unnecessary classes

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/ExternalLinkSlide.java

jdamerow · 2024-06-20T18:05:54Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/Slide.java

+
+    @OneToOne(targetEntity = VSImage.class)
+    @NotFound(action = NotFoundAction.IGNORE)
+    private IVSImage image;


this should not be needed. slides don't have images

jdamerow · 2024-06-20T18:07:37Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/IExternalLinkManager.java

@@ -2,6 +2,7 @@

 import edu.asu.diging.vspace.core.exception.ImageCouldNotBeStoredException;
 import edu.asu.diging.vspace.core.exception.LinkDoesNotExistsException;
+import edu.asu.diging.vspace.core.exception.SlideDoesNotExistException;


is this needed in this class?

jdamerow · 2024-06-20T18:08:44Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/ISlideManager.java

@@ -33,4 +33,7 @@ public interface ISlideManager {
    List<Sequence> getSlideSequences(String slideId, String moduleId);

    Page<ISlide> findByNameOrDescription(Pageable requestedPage,String searchText);
+
+    void storeSlideDisplay(ISlide slide, IVSImage image);


what is this for? should not be needed for this ticket.

jdamerow · 2024-06-20T18:09:46Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExternalLinkManager.java

+    @Override
+    protected IExternalLinkDisplay createDisplayLink(IExternalLink link) {
+        return null;
+    }


what is this all about? this file should not need to be changed.

jdamerow · 2024-06-20T18:31:09Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+        		$('#addChoice').show();
+        		$('#choiceSpace').show();
+        	[/]
+


what is all this added for?

jdamerow · 2024-06-20T18:31:42Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

Please fix this file so it's clear what has been changed. Right now i can't tell.

jdamerow · 2024-06-20T18:32:18Z

vspace/src/main/webapp/WEB-INF/views/staff/spaces/space.html

why is this file being changed?

jdamerow · 2024-06-20T18:33:02Z

...c/test/java/edu/asu/diging/vspace/core/factory/impl/SlideExternalLinkDisplayFactoryTest.java

+import edu.asu.diging.vspace.core.model.display.ISlideExternalLinkDisplay;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SlideExternalLinkDisplayFactoryTest {


There shouldn't be a ExternalLinkDisplayFactory anymore

jdamerow · 2024-06-20T18:33:36Z

vspace/src/test/java/edu/asu/diging/vspace/core/services/impl/SlideExternalLinkManagerTest.java

+        slideImage.setWidth(1300);
+        slide.setImage(slideImage);
+
+        ISlideDisplay displayAttributes = new SlideDisplay();


there should be no such class

jdamerow · 2024-10-22T20:32:11Z

vspace/src/main/java/edu/asu/diging/vspace/core/data/SlideExternalLinkRepository.java

+
+    public List<IExternalLinkSlide> findBySlide_Id(String slideId);
+
+    public void deleteByExternalLink(IExternalLinkSlide link);


just deleteByExternalLink. I don't think we need this method.

jdamerow · 2024-10-22T20:33:02Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/IExternalLinkManager.java

@@ -2,6 +2,7 @@

 import edu.asu.diging.vspace.core.exception.ImageCouldNotBeStoredException;
 import edu.asu.diging.vspace.core.exception.LinkDoesNotExistsException;
+import edu.asu.diging.vspace.core.exception.SlideDoesNotExistException;


jdamerow · 2024-10-22T20:36:01Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExternalLinkManager.java

+    @Override
+    protected IExternalLinkDisplay createDisplayLink(IExternalLink link) {
+        return null;
+    }


jdamerow · 2024-10-22T20:37:26Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/SlideExternalLinkManager.java

+    }
+
+    @Override
+    public void removeFromLinkList(ISlide slide, IExternalLinkSlide link) {


does this really need a method? when you want to remove a link, you can just do it directly without calling this extra method.

jdamerow · 2024-10-22T20:38:33Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/SlideExternalLinkManager.java

+    }
+
+    @Override
+    public void addToLinkList(ISlide slide, IExternalLinkSlide link) {


does this really need a method? when you want to add a link, you can just do it directly without calling this extra method.

jdamerow · 2024-10-22T20:41:56Z

vspace/src/main/java/edu/asu/diging/vspace/web/exhibit/view/ExhibitionSlideController.java

@@ -135,6 +136,7 @@ public String slide(Model model, @PathVariable("slideId") String slideId, @PathV
        model.addAttribute("currentNumOfSlide", slideIndex + 1);
        model.addAttribute("spaceId", spaceId);
        model.addAttribute("spaceName", spaceManager.getSpace(spaceId).getName());
+        model.addAttribute("slideExternalLinkList", slideExternalLinkManager.getLinks(slideId));


jdamerow · 2024-10-22T20:42:01Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/AddExternalLinkController.java

@@ -16,6 +16,7 @@
 import com.fasterxml.jackson.databind.node.ObjectNode;

 import edu.asu.diging.vspace.core.exception.ImageCouldNotBeStoredException;
+import edu.asu.diging.vspace.core.exception.SlideDoesNotExistException;


jdamerow · 2024-10-22T20:42:39Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/DeleteLinkController.java

@@ -41,4 +45,10 @@ public ResponseEntity<String> deleteExternalLink(@PathVariable("id") String spac
        externalLinkManager.deleteLink(linkId);
        return new ResponseEntity<String>(HttpStatus.OK);
    }
+
+    @RequestMapping(value = "/staff/module/{id}/slide/{slideId}/externallink/{linkId}", method = RequestMethod.DELETE)
+    public ResponseEntity<String> deleteSlideExternalLink(@PathVariable("id") String moduleId, @PathVariable("slideId") String slideId, @PathVariable("linkId") String linkId) {


jdamerow · 2024-10-22T20:43:19Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/EditSlideExternalLinkController.java

+
+    }
+
+    public ResponseEntity<String> checkIfSlideExists(ISlideManager slideManager, String id) throws IOException{


this extra method seems unnecessary

jdamerow · 2024-10-22T20:43:43Z

...c/test/java/edu/asu/diging/vspace/core/factory/impl/SlideExternalLinkDisplayFactoryTest.java

+import edu.asu.diging.vspace.core.model.display.ISlideExternalLinkDisplay;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SlideExternalLinkDisplayFactoryTest {


DeleteLinkController (see comment)

…aces-2.0 into story/VSPC-242 "divergent local and repo branches"

jophals · 2024-10-31T20:31:14Z

i had do to multiple merges to fix conflicts and diverging

jdamerow · 2024-11-28T03:07:13Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/DeleteSpaceLinkController.java

@@ -13,7 +13,7 @@
 import edu.asu.diging.vspace.core.services.ISpaceLinkManager;

 @Controller
-public class DeleteLinkController {
+public class DeleteSpaceLinkController {


That's not correct, is it?

What do you mean? I renamed it DeleteSpaceLinkController because it was housing methods for deleting module slide links and space links and you didn't want to mix them. So now there's DeleteSpaceLinkController and DeleteSlideLinkController

do you want me to just keep the name as DeleteLinkController?

yes, this class does not just delete spacelinks, so the name is misleading.

oh okay, got it. I named it that because they all originate from '/staff/space'. But I see what you mean

jdamerow · 2024-11-28T03:07:49Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/SpaceController.java

@@ -78,6 +79,7 @@ public ResponseEntity<Map<String,Object>> showSpaceLinks(@PathVariable String id
        Map<String,Object> responseData = new HashMap<String,Object>();
        responseData.put("spaceLinks", spaceLinkManager.getLinkDisplays(id));
        responseData.put("externalLinks", externalLinkManager.getLinkDisplays(id));
+        System.out.println("EXTERNAL LINKS 2: " + externalLinkManager.getLinkDisplays(id));
        responseData.put("moduleLinks", moduleLinkManager.getLinkDisplays(id));


remove sysouts

jdamerow · 2024-11-28T03:08:46Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

controllers, cleaned up some unused code in slide.html, and removed comments/print statements

…aces-2.0 into story/VSPC-242 Merging due to mismatch with remote branch

swetalina-prusty25 added 30 commits August 7, 2023 16:21

[story/VSPC-242]UI changes

1f5cc4a

[story/VSPC-242]create external link ajax call

ff64e16

[story/VSPC-242]external link alert UI

4ae81d8

[story/VSPC-242]Javascript

0225cc2

[story/VSPC-242] Edit external link

29725e7

[story/VSPC-242] drawlink

954d2fb

[story/VSPC-242]Entity

c165ac5

[story/VSPC-242]Entity

983cf14

[story/VSPC-242]Factory Class

ef6194e

[story/VSPC-242]Repo

b4e6a32

[story/VSPC-214] create external link

e079705

[story/VSPC-242] Controller

bc6c35a

[story/VSPC-242] updated controller

a4106ee

[story/VSPC-242]create link

800e222

[story/VSPC-242]Create and display link

91257f7

[story/VSPC-242]Delete link

fe88ded

[story/VSPC-242]update and delete

a5f4d2c

[story/VSPC-242]controller

26406a0

[story/VSPC-242]set positions

93e1bae

[story/VSPC-242] Java script

726ca20

[story/VSPC-242]Utility js functions

d49a364

[story/VSPC-242]External link editable

158ac5a

[story/VSPC-242] Text area function

3713c45

[story/VSPC-242]Buttons ajax call

d14bdcb

[story/VSPC-242] Ajax method for create link

89cae0d

[story/VSPC-242]Ajax method for edit external link

bc9c564

[story/VSPC-242] Delete button java script

62906bf

[story/VSPC-242] Delete and cancel button

04f9c0a

[story/VSPC-242] Fixing front end issue

598e51a

[story/VSPC-242] Alert issue fixed

d5efaa7

jophals reopened this Jun 12, 2024

[VSPC-242] fixed findBySlide query

b4bf072

jdamerow requested changes Jun 13, 2024

View reviewed changes

jdamerow closed this Jun 13, 2024

[VSPC-242] fixed SlideExternalLink repo, unnecessary spacing, imports,

9a3567a

slide.html errors, and deleted unnecessary classes

jophals reopened this Jun 14, 2024

jdamerow requested changes Jun 20, 2024

View reviewed changes

jdamerow closed this Jun 20, 2024

made PR fixes; tried to restore some files to previous state

fbb4f6e

jophals reopened this Jul 18, 2024

jophals requested a review from jdamerow July 18, 2024 21:13

Merge branch 'develop' into story/VSPC-242

5cb787e

jdamerow requested changes Oct 22, 2024

View reviewed changes

jdamerow closed this Oct 22, 2024

jophals added 4 commits October 30, 2024 16:38

Executed suggested PR changes except the changes for

5144ae0

DeleteLinkController (see comment)

[VSPC-242] implemented additional PR change like DeleteLinkController

204c91b

[VSPC-242] additional PR comment fixes

23b575a

Merge branch 'story/VSPC-242' of https://github.com/diging/virtual-sp…

13e9bd5

…aces-2.0 into story/VSPC-242 "divergent local and repo branches"

jophals reopened this Oct 31, 2024

Merge branch 'develop' into story/VSPC-242

b12f048

jdamerow requested changes Nov 28, 2024

View reviewed changes

jdamerow closed this Nov 28, 2024

jophals added 3 commits December 13, 2024 11:08

[VSPC-242] implemented PR changes: fixed naming issue with link

293df7a

controllers, cleaned up some unused code in slide.html, and removed comments/print statements

Merge branch 'story/VSPC-242' of https://github.com/diging/virtual-sp…

f0a0aa3

…aces-2.0 into story/VSPC-242 Merging due to mismatch with remote branch

[story/VSPC-242] link controller name fix

9d24363

jophals self-assigned this Dec 13, 2024

jophals reopened this Dec 13, 2024

		@@ -0,0 +1,12 @@
		package edu.asu.diging.vspace.core.model;

		public interface ILinkSlide<T extends IVSpaceElement> extends IVSpaceElement {


		import edu.asu.diging.vspace.core.model.IExternalLinkSlide;

		public interface ISlideExternalLinkDisplay extends ILinkDisplay {


		public List<IExternalLinkSlide> findBySlide_Id(String slideId);

		public void deleteByExternalLink(IExternalLinkSlide link);


		}

		public ResponseEntity<String> checkIfSlideExists(ISlideManager slideManager, String id) throws IOException{

story/VSPC-242 #345

Are you sure you want to change the base?

story/VSPC-242 #345

Conversation

jophals commented Jun 7, 2024

Guidelines for Pull Requests

Please provide a brief description of your ticket

Anything else the reviewer needs to know?

jophals commented Jun 13, 2024

jophals commented Jun 13, 2024

jdamerow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jophals commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment