Story/vspc 204 #308

pkharge · 2022-07-29T23:35:08Z

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

The user would get a zip file that contains one html page for each page + any css, javascript, etc.

Anything else the reviewer needs to know?

... describe here ...

diging-jenkins · 2022-07-29T23:35:12Z

Can one of the admins verify this patch?

jdamerow

I didn't not go through everything but given I'm asking for some major changes, I'll leave it with this for now.

jdamerow · 2022-09-06T20:12:30Z

vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java

+	        });
+            IOUtils.close(responseZipStream);
+            IOUtils.close(bufferedOutputStream);
+            IOUtils.close(byteArrayOutputStream);


this can probably be tightened up by using try-with statements

jdamerow · 2022-09-06T20:12:34Z

vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java

+	                responseZipStream.closeEntry();
+
+	            } catch (IOException e) {
+	                System.err.println(e);


jdamerow · 2022-09-06T20:13:15Z

vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java

+            return byteArrayOutputStream.toByteArray();
+
+        } catch (IOException e) {
+            throw new IOException(e);


there is no point in wrapping an IOException in another IOException

vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java

jdamerow · 2022-09-06T20:14:01Z

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

+
+
+@Service
+public class DownloadsManager {


needs an interface

From what I can tell so far, this class needs to be a little refactored. There should be several methods:

create a snapshot (or whatever you want to call the generated html pages)

get one

list all

get generated files

When the user generates a new snapshot the create method is called. This should be an @Async method as it might take a while. The user sees a progress page (polling for updates from the front end, not just one call as the server might time out).
When the generation process is done, the user will be sent to the page for that snapshot. The create method should return the created ExhibitionDownload object. The get method is probably called here, to get the object by its id. If the user wants to download the files, they can do so from that page (get generated files method will be called).
The list all snapshots page will call the list method.

@jdamerow , want to check my understanding on this once.
The first time user clicks on download button, the async rest api will return an newly created exhibitionDownload object with a unique folder name (used timestamps). this api will trigger create snapshot method.
then Ui will poll another rest api (this will be an simple GET api) by sending exhibitionDownload object as input, untill the requested folder is ready. once its ready, the folder should be download as zip folder.

so there will be 2 different rest apis in backend, one async which just returns exhibitionDownload object and trigger createSnapshot method. another one will be used simply to poll.

is this understanding correct?

Not 100% sure. What do you mean by "async rest api"?

The flow would be:

POST request to start zip generation (the request itself does not have to be async, but it will start an async method on the server)

the controller will return an id that the client can then use to poll (id could be the id of an exhibitionDownload object if that is being created and returned by the async method)

"then Ui will poll another rest api (this will be an simple GET api)" - yes, "by sending exhibitionDownload object as input" -> not sure what that means. It would use the id returned in the previous step

"untill the requested folder is ready." - yes

"once its ready, the folder should be download as zip folder." - once it's ready the user should be able to down load, it should probably show a message that the generation is done or something. It does not have to automatically start downloading.

@jdamerow , to show the progress of download to the user, do we want to create a separate page where user will be redirected to, or can we show it within a section in the same downloadlist page?
I tried putting it in an alert box, but if the page gets refreshed, the user won't be able to see the notification.

either is fine.

if it's on the same page and the user won't see the progress anymore after reloading the page, there is needs to be another indication that makes clear that export is in progress.

vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java

jdamerow · 2022-10-24T19:36:45Z

vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java

+	        });
+            IOUtils.close(responseZipStream);
+            IOUtils.close(bufferedOutputStream);
+            IOUtils.close(byteArrayOutputStream);


are these needed since you're using the try-with statement?

vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java

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

jdamerow · 2022-10-24T19:40:34Z

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

+        Resource resource = null; 
+        try {     
+            String pathToResources = request.getServletContext().getRealPath("") + "/resources";
+            String exhibitionFolderName= "Exhibition"+ LocalDateTime.now();


business logic that should not be in the controller

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

Resolve conflicts please

# Conflicts: # vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/SpaceManager.java

jdamerow · 2025-01-10T15:58:40Z

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

+        try {
+            snapshotTask = createSnapshot(resourcesPath, exhibitionFolderName, sequenceHistory, exhibitionSnapshot);
+            storageEngineDownloads.generateZip(exhibitionFolderName);           
+            snapshotTask.setTaskComplete(true);


wait, I'm confused. createSnapshot spins up a new thread, right? so you can't generate a zip file right after you call it, can you? and it also wont' be complete right after, doesn't it? Or am I missing something?

wait, actually, if this method calls createSnapshot, it won't spin up a new thread. It should though, since it could take quite a while.

This still. createSnapshot is annotated with@Async but if called from within the class, no new thread is created.

jdamerow · 2025-04-02T16:33:18Z

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

+import edu.asu.diging.vspace.core.services.ISnapshotManager;
+
+@Service
+@EnableAsync


This should not be necessary. This is an annotation for a configuration class.

jdamerow · 2025-04-02T16:35:04Z

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

+        exhibitionSnapshot = exhibitionSnapshotRepository.save(exhibitionSnapshot);
+
+        try {
+            snapshotTask = createSnapshot(resourcesPath, exhibitionFolderName, sequenceHistory, exhibitionSnapshot);


sequenceHistory never seems to be initialized?

also, createSnapshot will not create a new thread as you are calling the method from within this class!

jdamerow · 2025-04-02T16:37:29Z

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

+        }
+        try {
+            return storageEngineDownloads.getMediaContent("",exhibitionSnapshot.get().getFolderName()+ZIP_FILE_EXTENSION);                
+        }catch(Exception e) {


is anything actually throwing Exception? If so, there should be a comment here. If not, then the specific exception thrown should be caught here.

jdamerow · 2025-04-02T16:37:47Z

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

+        }
+        Pageable requestedPageForFiles = PageRequest.of(filesPagenum - 1, pageSize);
+
+        Page<ExhibitionSnapshot> page =   exhibitionSnapshotRepository.findAllByOrderByCreationDateDesc(requestedPageForFiles);


jdamerow · 2025-04-02T16:39:26Z

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

+    public ResponseEntity<SnapshotTask> getSnapshotTaskStatus(HttpServletRequest request,
+            @PathVariable(required = false, name = "id") String snapshotId, HttpServletResponse response, Model model)
+            throws ExhibitionSnapshotNotFoundException {
+        SnapshotTask snapshotTask = (snapshotId == null && snapshotId == "")


you don't compare strings with ==

also how can a variable be null and also be ""?

jdamerow · 2025-04-02T16:40:27Z

also, there is a conflict

# Conflicts: # vspace/src/test/java/edu/asu/diging/vspace/core/services/impl/ImageServiceTest.java

jdamerow · 2025-06-17T18:14:33Z

Make it so, Jenkins.

jdamerow · 2025-06-17T18:22:15Z

A couple of issues:

After clicking "Create Snapshot" nothing happens. There should be some indication that the snapshot generation has started. And if a snapshot generation is currently running, there needs to be some indication on the page as well.
The exported exhibition:
- there is not index.html file in the generated root directory, so it's not clear what file is the first page of the exhibition
- there are no links on any of the pages apparently! So one cannot move from one page to the next.

jdamerow · 2025-06-17T18:22:44Z

vspace/pom.xml

        <uploaded.files.path></uploaded.files.path>
-
+        <downloaded.files.path></downloaded.files.path>
+        <resources.path></resources.path>


these two need a comment what should go here.

jdamerow · 2025-06-17T18:23:35Z

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

+     */   
+    @Async
+    @Transactional
+    public Future<SnapshotTask> createSnapshot(String resourcesPath, String exhibitionFolderName,SequenceHistory sequenceHistory, ExhibitionSnapshot exhibitionSnapshot) 


ideally, resource path would not be required but calculate.

jdamerow · 2025-06-17T18:23:51Z

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

+import edu.asu.diging.vspace.core.services.IRenderingManager;
+
+@Service
+public class AsyncSnapshotCreator {


needs an interface

…ng links

Prachi Kharge added 18 commits May 26, 2022 16:12

[story/VSPC-204] added download section in staff page

0783f6d

[story/VSPC-204] download api

d1b15cc

[story/VSPC-204] added jsoup dependency

e1cda05

[story/VSPC-204] modified download api response

d68de99

[story/VSPC-204] copy images to new folder structure

9e509e2

[story/VSPC-204] store images under eaach space

70907bb

[story/VSPC-204] downlaod template modified to pick up images

0d2e771

[story-VSPC-204] copied resources for css

78c5003

[story/VSPC-204] converted to zip folder. added template for slides

1a6da12

[story/VSPC-204] added code to consider branching points

7a71f63

[story/VSPC-204] zip folder download working as expected

737dad5

[story/VSPC-204] list of downlods

0a24c03

[story/VSPC-204] refactored downloads manager

ef2ee5a

[story/VSPC-204] refactor

0c3e206

[story/VSPC-204] test cases

3caf863

[story/VSPC-204] test cases

b7d9442

[story/VSPC-204] removed controller call to download html template

34496eb

[story/VSPC-204] refactor

e74fe71

[story/VSPC-204]

e280828

pkharge marked this pull request as draft July 29, 2022 23:40

Prachi Kharge added 3 commits August 1, 2022 15:26

[story/VSPC-204] removed unwanted methods

dff8c3d

[story/VSPC-204] code factor

e1bf075

[story/VSPC-204] code factor

3adad85

pkharge marked this pull request as ready for review August 1, 2022 23:16

Prachi Kharge added 2 commits August 1, 2022 16:18

[story/VSPC-204]

3a86887

Merge branch 'develop' into story/VSPC-204

d2a9a14

jdamerow requested changes Oct 24, 2022

View reviewed changes

jdamerow closed this Oct 24, 2022

[story/VSPC-204] review comments

3926ce1

pooja-thalur reopened this Nov 12, 2024

jdamerow closed this Jan 6, 2025

Merge branch 'develop' into story/VSPC-204

27bcf6b

# Conflicts: # vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/SpaceManager.java

pooja-thalur reopened this Jan 7, 2025

jdamerow reviewed Jan 10, 2025

View reviewed changes

jdamerow closed this Feb 7, 2025

pooja-thalur added 2 commits March 26, 2025 16:37

[VSPC-204] Async changes

3d19a1e

[VSPC-204] failing testcase fixed

79210a4

pooja-thalur reopened this Apr 2, 2025

jdamerow requested changes Apr 2, 2025

View reviewed changes

jdamerow closed this Apr 2, 2025

pooja-thalur added 4 commits April 9, 2025 16:36

[VSPC-204] review comments addressed

93b1aa9

[VSPC-204] async changes

d665d9e

[VSPC-204] fixed unit tests

a205132

[VSPC-204] code cleanup

5f1b1cc

pooja-thalur reopened this Apr 15, 2025

pooja-thalur added 2 commits April 15, 2025 11:06

Merge branch 'develop' into story/VSPC-204

41d73db

# Conflicts: # vspace/src/test/java/edu/asu/diging/vspace/core/services/impl/ImageServiceTest.java

[VSPC-204] upload storage engine change

4256d9e

jdamerow closed this Jun 17, 2025

jdamerow reviewed Jun 17, 2025

View reviewed changes

Girik1105 added 5 commits October 3, 2025 16:26

[VSPC-204] Fixed ui issues, proper nav for exhibitions exports, worki…

2fa6b8c

…ng links

[VSPC-204] Added AsyncSnapshotCreator interface

937227c

[VSPC-204] Pom update

d7c3f76

Merge branch 'develop' into story/VSPC-204

f1407a2

[VSPC-204] Fixed var cannot be resolved to type issue

c66eac7

Girik1105 reopened this Oct 6, 2025

Story/vspc 204 #308

Are you sure you want to change the base?

Story/vspc 204 #308

Uh oh!

Conversation

pkharge commented Jul 29, 2022

Guidelines for Pull Requests

Please provide a brief description of your ticket

Anything else the reviewer needs to know?

Uh oh!

diging-jenkins commented Jul 29, 2022

Uh oh!

jdamerow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdamerow commented Jan 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdamerow commented Apr 2, 2025

Uh oh!

jdamerow commented Jun 17, 2025

Uh oh!

jdamerow commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers