Story/vspc 204 #308
Story/vspc 204 #308
Conversation
|
Can one of the admins verify this patch? |
| }); | ||
| IOUtils.close(responseZipStream); | ||
| IOUtils.close(bufferedOutputStream); | ||
| IOUtils.close(byteArrayOutputStream); |
There was a problem hiding this comment.
this can probably be tightened up by using try-with statements
| responseZipStream.closeEntry(); | ||
|
|
||
| } catch (IOException e) { | ||
| System.err.println(e); |
| return byteArrayOutputStream.toByteArray(); | ||
|
|
||
| } catch (IOException e) { | ||
| throw new IOException(e); |
There was a problem hiding this comment.
there is no point in wrapping an IOException in another IOException
vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java
Show resolved
Hide resolved
|
|
||
|
|
||
| @Service | ||
| public class DownloadsManager { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
| }); | ||
| IOUtils.close(responseZipStream); | ||
| IOUtils.close(bufferedOutputStream); | ||
| IOUtils.close(byteArrayOutputStream); |
There was a problem hiding this comment.
are these needed since you're using the try-with statement?
vspace/src/main/java/edu/asu/diging/vspace/core/file/impl/StorageEngine.java
Show resolved
Hide resolved
vspace/src/main/java/edu/asu/diging/vspace/core/model/IExhibitionDownload.java
Outdated
Show resolved
Hide resolved
| Resource resource = null; | ||
| try { | ||
| String pathToResources = request.getServletContext().getRealPath("") + "/resources"; | ||
| String exhibitionFolderName= "Exhibition"+ LocalDateTime.now(); |
There was a problem hiding this comment.
business logic that should not be in the controller
| * @param sequenceHistory the sequence history object having the history of sequences | ||
| * @throws FileStorageException | ||
| */ | ||
| public void downloadSpace(Space space, String exhibitionFolderName, SequenceHistory sequenceHistory) throws FileStorageException { |
There was a problem hiding this comment.
this method is not downloading anything, is it? Just creating a snapshot/rendering the pages?
| * @param spaceFolderPath he path of the folder where the module's content will be stored | ||
| * | ||
| */ | ||
| private void downloadModule(IModule module, ISpace space, String imagesFolderPath, String spaceFolderPath) { |
| * @throws FileStorageException | ||
| * | ||
| */ | ||
| private void downloadSequences(ISequence startSequence, IModule module, ISpace space, String spaceFolderPath, |
| if(slide instanceof BranchingPoint) { | ||
| BranchingPoint branchingPoint = (BranchingPoint) slide; | ||
| List<IChoice> choices = branchingPoint.getChoices(); | ||
| choices.forEach(choice -> { |
There was a problem hiding this comment.
these three lines could be merged into one, or at least 2.
vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/RenderingManager.java
Show resolved
Hide resolved
| public SnapshotTask getSnapshotTask(String id) throws ExhibitionSnapshotNotFoundException{ | ||
| Optional<SnapshotTask> snapshotTask = snapshotTaskRepository.findById(id); | ||
| if(snapshotTask.isPresent()) { | ||
| logger.debug("hereee"); |
| } | ||
|
|
||
| @RequestMapping(value = "/staff/exhibit/download/{id}", method = RequestMethod.GET) | ||
| public ResponseEntity<Resource> downloadExhibitionFolder(@PathVariable("id") String id, @RequestParam("folderName") String exhibitionSnapshotFolderName , HttpServletRequest request) |
There was a problem hiding this comment.
should be in its own controller
| } | ||
|
|
||
| @RequestMapping(value = "/staff/exhibit/snapshot", method = RequestMethod.POST) | ||
| public ResponseEntity<ExhibitionSnapshot> createExhibitionSnapshot(HttpServletRequest request, HttpServletResponse response, Model model) { |
There was a problem hiding this comment.
should be in its own controller
|
|
||
|
|
||
| @RequestMapping(value = "/staff/exhibit/download/checkStatus/{id}", method = RequestMethod.GET) | ||
| public ResponseEntity<Boolean> exhibitionDownloadStatus(@PathVariable("id") String id, @RequestParam("folderName") String exhibitionDownloadFolderName , HttpServletRequest request) throws ExhibitionSnapshotNotFoundException { |
There was a problem hiding this comment.
should probably be in the controller with createExhibitionSnapshot
There was a problem hiding this comment.
wait, what's the difference between this endpoint and getLatestSnapshotStatus? Would one method returning the status of a task not be enough?
| public class SnapshotTaskController { | ||
|
|
||
| @Autowired | ||
| ISnapshotManager snapshotManager; |
| * @throws IOException | ||
| */ | ||
| @Override | ||
| public byte[] getZip(String zipFilename) throws ExhibitionSnapshotNotFoundException, IOException, FileNotFoundException { |
There was a problem hiding this comment.
Hm, is this needed? Why can't it use the getFile method?
| public byte[] getZip(String zipFilename) throws ExhibitionSnapshotNotFoundException, IOException, FileNotFoundException { | ||
| File file = new File(path + File.separator + zipFilename + ".zip"); | ||
| if(!file.exists()){ | ||
| throw new ExhibitionSnapshotNotFoundException(zipFilename); |
There was a problem hiding this comment.
the storage engine shouldn't know anything about what a file is for, so it should not through a snapshot exception. If this method is needed (and getFile can't be used), then it should be a generic method that could be use for any file (e.g. make it getFileBytes or something).
There was a problem hiding this comment.
But also, there is getMediaContent, which also seems to do more or less the same thing
| createSlideSnapshot(slide, sequence, module, space, spaceFolderName, imagesFolderName); | ||
| if(slide instanceof BranchingPoint) { | ||
| ((BranchingPoint) slide).getChoices().forEach(choice -> { | ||
| if(!choice.getSequence().getId().equals(sequence.getId())) { |
There was a problem hiding this comment.
I think there needs to be a "bigger" check. The linked sequence could have a branching point that links back to the first sequence, that would still create an infinite loop here, wouldn't it?
| context.setVariable("startSequenceId", startSequenceId); | ||
| ISequence sequenceExist=moduleManager.checkIfSequenceExists(moduleId, sequenceId); | ||
| if (sequenceExist==null) { | ||
| throw new SequenceNotFoundException(sequenceId); |
| ExhibitionSnapshot exhibitionSnapshot = new ExhibitionSnapshot(); | ||
| createSnapshotFolder(exhibitionSnapshot, exhibitionFolderName); | ||
| SnapshotTask snapshotTask = createSnapshotTask(exhibitionSnapshot); | ||
| exhibitionSnapshot.setSnapshotTask(snapshotTask); |
There was a problem hiding this comment.
this method should set the task complete after the zip has been generated.
|
|
||
| if(exhibitionSnapshot.isPresent()) { | ||
| try { | ||
| return storageEngineDownloads.getZip(exhibitionSnapshot.get().getFolderName()); |
There was a problem hiding this comment.
what if the snapshot generation is not done yet? there shouldn't be a not found exception in that case.
| @Autowired | ||
| private ISnapshotManager snapshotManager; | ||
|
|
||
| @RequestMapping("/staff/downloads/list") |
|
|
||
| private final Logger logger = LoggerFactory.getLogger(getClass()); | ||
|
|
||
| @RequestMapping(value = "/staff/exhibit/snapshot", method = RequestMethod.POST) |
| } | ||
| } | ||
|
|
||
| @RequestMapping(value = "/staff/exhibit/download/{id}", method = RequestMethod.GET) |
|
|
||
| private final Logger logger = LoggerFactory.getLogger(getClass()); | ||
|
|
||
| @RequestMapping(value = "/staff/exhibit/snapshot/status", method = RequestMethod.GET) |
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]).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 ...