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

# 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

jdamerow · 2025-10-15T18:34:08Z

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

+        htmlContent.append("        </div>\n");
+        htmlContent.append("    </div>\n");
+        htmlContent.append("</body>\n");
+        htmlContent.append("</html>");


this should be done using a template like the rest of the files.

jdamerow · 2025-10-15T18:34:16Z

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) 


jdamerow · 2025-10-15T18:35:52Z

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

+                Set<String> visitedSequences = new HashSet<String>();
+                createSequencesSnapshot(startSequence, module, space, spaceFolderName, imagesFolder, visitedSequences);
+            } catch (FileStorageException e) {
+                logger.error("Could not download Module",e);


error like this need to be communicated to the user; they need to know if a snapshot couldn't fully be created.

jdamerow · 2025-10-15T18:40:35Z

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

+     * @param context
+     * @param id
+     */
+    private void populateContextForSpace(Context context, String id, SequenceHistory sequenceHistory) {


it seems like sequenceHistory is never used?

jdamerow · 2025-10-15T18:41:18Z

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

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


this doesn't seem to make sense; if sequenceHistory is autowired, wouldn't it get the history of the current user? that should not be used to generate a snapshot. I don't think sequence history is at important for snapshots?

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

… indexDownloadTemplate html file for dynamic downloads

…d code style, updated interface

jdamerow · 2025-10-27T15:55:44Z

vspace/pom.xml

-
+        <uploaded.files.path>${project.basedir}/uploads</uploaded.files.path>
+        <downloaded.files.path>${project.basedir}/downloads</downloaded.files.path>
+        <resources.path>${project.basedir}/src/main/webapp/resources</resources.path>


I believe project.basedir is an absolute path to the base directory of the project? This does not work, as the war will be build on a different system than where it is deployed, so the directory will be a different one. where ever those variables (uploaded.files.path, etc) are being used, it should use relative paths for directories that are within the project, and absolute paths set through configurations for folders outside of the project..

jdamerow · 2025-10-27T15:58:08Z

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

-        byte[] bytes = byteOutput.toByteArray();
-        byteOutput.close();
-        return bytes;
+        catch(IOException e) {


should be on line 106 according to our style guide

jdamerow · 2025-10-27T15:59:17Z

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

+                    throw new IOException(e.getMessage(), e);
+                }
+            }
+            // Delete the folder


useless comment; comments should explain why not what (unless it's not obvious from the code, which it is here).

jdamerow · 2025-10-27T16:02:18Z

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

+            // Delete the folder
+            deleteFolder(folderPath, folderName); 
+        } catch (IOException e) {
+            // Delete the zip file if an exception occurred


jdamerow · 2025-10-27T16:04:49Z

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

+        // Get exhibition details
+        IExhibition exhibition = exhibitionManager.getStartExhibition();
+        String exhibitionTitle = exhibition != null ? exhibition.getTitle() : "Virtual Exhibition";
+        Space startSpace = exhibition != null && exhibition.getStartSpace() != null ? 


Why not use the interface here instead of Space?

jdamerow · 2025-10-27T16:16:08Z

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

+        try {
+            return storageEngineDownloads.getMediaContent("", exhibitionSnapshot.get().getFolderName() + ZIP_FILE_EXTENSION);                
+        } catch (IOException e) {
+            throw new ExhibitionSnapshotNotFoundException(e.getMessage(), e);


if you just throw the exception, there is no reason to repeat the message of the wrapped exception. Either have a message that explains something or just not provide a message at all (and just wrap the exception).

jdamerow · 2025-10-27T16:18:31Z

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);
+        return page.map(exhibitionSnapshot-> {return (ExhibitionSnapshot) exhibitionSnapshot;});


I'm confused, what is this line doing? looks like it's mapping snapshots onto themselves?

jdamerow · 2025-10-27T16:18:47Z

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

+        if(snapshotTask.isPresent()) {
+            return snapshotTask.get();
+        }
+        else {


jdamerow · 2025-10-27T16:21:44Z

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

+    	$('.successAlert').append(alert);
+    	const poll = (id, folderName) => {
+            maxAttempts = 10;
+            interval= 3000;


is that miliseconds? (needs a comment)

jdamerow · 2025-10-27T16:22:17Z

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

+    	var alert = $('<div class="alert alert-info alert-dismissible fade show" role="alert"><p>Snapshot '+folderName+' creation is in progress. Please wait. </p><button type="button" class="close" data-dismiss="alert" aria-label="Close"><span aria-hidden="true">&times;</span></button></div>');
+    	$('.successAlert').append(alert);
+    	const poll = (id, folderName) => {
+            maxAttempts = 10;


this seems odd. Why would polling for a completed task have a maximum number of attempts. It should poll until complete or there was an error.

… paths, added comments, removed useless comments, fixed error messages, added ISpace iterface in AysncSnapShotCreator

…nd development, fixed UI issue

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

[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

jdamerow requested changes Oct 15, 2025

View reviewed changes

jdamerow closed this Oct 15, 2025

Girik1105 added 3 commits October 16, 2025 11:59

[VSPC-204] Instead if manually using asyncSnapshotCreator, made a new…

66c2df6

… indexDownloadTemplate html file for dynamic downloads

[VSPC-204] Removed sequence history paramater, Improved javadoc, fixe…

8badb86

…d code style, updated interface

[VSPC-204] Downloads install if not exists

c8f23d7

Girik1105 reopened this Oct 24, 2025

[VSPC-204] Merged develop

55675ba

jdamerow requested changes Oct 27, 2025

View reviewed changes

jdamerow closed this Oct 27, 2025

Girik1105 added 5 commits November 6, 2025 12:01

[VSPC-204] Fixed issues

cb5b087

[VSPC-204] PR comments Replace hardcoded absolute paths with relative…

9acaf33

… paths, added comments, removed useless comments, fixed error messages, added ISpace iterface in AysncSnapShotCreator

[VSPC-204] PR comments

8cabe8c

Merge branch 'develop' into story/VSPC-204

7116711

[VSPC-204] Fixed enviornment variable issues, set it for production a…

42bc100

…nd development, fixed UI issue

Girik1105 reopened this Nov 25, 2025

Merge branch 'develop' into story/VSPC-204

bed6562

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

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!

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!