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!

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

jdamerow · 2025-01-06T16:26:35Z

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

+
+@Repository
+@JaversSpringDataAuditable
+public interface SlideExternalLinkRepository extends PagingAndSortingRepository<ExternalLinkSlide, String> {


This repo is called SlideExternalLinkRepository but the model is called ExternalLinkSlide. Should be consistentn

jdamerow · 2025-01-06T16:26:47Z

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

+
+@Transactional
+@Service
+public class SlideExternalLinkManager


are you saying that this needs a Javadoc as well or that there is a naming inconsistency similar to above?

jdamerow · 2025-01-06T16:28:22Z

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

+    }
+
+    @Override
+    public IExternalLinkSlide createExternalLink(String label, String url, String slideId) {


there needs to be javadoc. Most importantly it needs to be documented that this method does not save the external link it creates, or does it?

jdamerow · 2025-01-06T16:30:45Z

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

+        return success(link.getId(), link.getExternalLink(), title, null, null);
+    }
+
+    public ResponseEntity<String> success(String id, String url, String label, String linkedId, String linkedSlideStatus) throws IOException{


this should not be public.

jdamerow · 2025-01-06T16:32:05Z

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

+            return new ResponseEntity<>("{'error': 'Slide could not be found.'}", HttpStatus.NOT_FOUND);
+        }
+        IExternalLinkSlide link = externalLinkManager.updateExternalLink(title, externalLink, id);
+        return success(link.getId(), link.getExternalLink(), title, null, null);


success is only called once, right? here? if so, there is really no need to have the last two parameters if they are always null.

jdamerow · 2025-01-06T16:32:34Z

also, there are conflicts

consistency throughout project, added Javadocs, cleaned up unused paramters...

…aces-2.0 into story/VSPC-242 needed to merge in order to push new changes

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 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

jdamerow requested changes Jan 6, 2025

View reviewed changes

jdamerow closed this Jan 6, 2025

jophals reopened this Jan 6, 2025

jophals and others added 3 commits January 7, 2025 12:21

Merge branch 'develop' into story/VSPC-242

dc7e1d4

[story/VSPC-242] implemented PR fixes: changing model name for

1fe4663

consistency throughout project, added Javadocs, cleaned up unused paramters...

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

485c418

…aces-2.0 into story/VSPC-242 needed to merge in order to push new changes


		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?

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

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

jdamerow commented Jan 6, 2025