Merge pull request #3754 from ControlSystemStudio/CSSTUDIO-3682

shroffk · web-flow · commit d31ca94f4dd4 · 2026-06-18T16:19:28.000-04:00
Support add/remove attachments when editing Olog entry
diff --git a/app/logbook/olog/client-es/src/main/java/org/phoebus/olog/es/api/OlogHttpClient.java b/app/logbook/olog/client-es/src/main/java/org/phoebus/olog/es/api/OlogHttpClient.java
@@ -32,6 +32,7 @@
 import org.phoebus.util.http.HttpRequestMultipartBody;
 import org.phoebus.util.http.QueryParamsHelper;
 
+import javax.ws.rs.HttpMethod;
 import javax.ws.rs.core.MultivaluedHashMap;
 import javax.ws.rs.core.MultivaluedMap;
 import java.io.InputStream;
@@ -158,7 +159,7 @@ private OlogHttpClient(String userName, String password) {
 
     @Override
     public LogEntry set(LogEntry log) throws LogbookException {
-        return save(log, null);
+        return save(log, null, HttpMethod.PUT);
     }
 
     /**
@@ -171,7 +172,7 @@ public LogEntry set(LogEntry log) throws LogbookException {
      * @throws LogbookException E.g. due to invalid log entry data, or if attachment content type
      *                          cannot be determined.
      */
-    private LogEntry save(LogEntry log, LogEntry inReplyTo) throws LogbookException {
+    private LogEntry save(LogEntry log, LogEntry inReplyTo, String method) throws LogbookException {
         try {
             javax.ws.rs.core.MultivaluedMap<String, String> queryParams = new javax.ws.rs.core.MultivaluedHashMap<>();
             queryParams.putSingle("markup", "commonmark");
@@ -183,24 +184,29 @@ private LogEntry save(LogEntry log, LogEntry inReplyTo) throws LogbookException
             httpRequestMultipartBody.addTextPart("logEntry", OlogObjectMappers.logEntrySerializer.writeValueAsString(log), "application/json");
 
             for (Attachment attachment : log.getAttachments()) {
-                httpRequestMultipartBody.addFilePart(attachment.getFile(), attachment.getUniqueFilename());
+                if(attachment.getFile() != null){
+                    httpRequestMultipartBody.addFilePart(attachment.getFile(), attachment.getUniqueFilename());
+                }
             }
 
+            HttpRequest.BodyPublisher bodyPublisher = HttpRequest.BodyPublishers.ofByteArray(httpRequestMultipartBody.getBytes());
+
             HttpRequest request = HttpRequest.newBuilder()
                     .uri(URI.create(Preferences.olog_url + OLOG_PREFIX + "/logs/multipart?" + QueryParamsHelper.mapToQueryParams(queryParams)))
                     .header("Content-Type", httpRequestMultipartBody.getContentType())
                     .header(OLOG_CLIENT_INFO_HEADER, CLIENT_INFO)
                     .header("Authorization", basicAuthenticationHeader)
-                    .PUT(HttpRequest.BodyPublishers.ofByteArray(httpRequestMultipartBody.getBytes()))
+                    .method(method, bodyPublisher)
+                    //.PUT(HttpRequest.BodyPublishers.ofByteArray(httpRequestMultipartBody.getBytes()))
                     .build();
 
             HttpResponse<String> response = httpClient.send(request, HttpResponse.BodyHandlers.ofString());
             if(response.statusCode() == 401){
-                LOGGER.log(Level.SEVERE, "Failed to create log entry: user not authenticated");
+                LOGGER.log(Level.SEVERE, "Failed to create or update log entry: user not authenticated");
                 throw new LogbookException(Messages.SubmissionFailedInvalidCredentials);
             }
             else if (response.statusCode() >= 300) {
-                LOGGER.log(Level.SEVERE, "Failed to create log entry: " + response.body());
+                LOGGER.log(Level.SEVERE, "Failed to create or update log entry: " + response.body());
                 throw new LogbookException(response.body());
             } else {
                 return OlogObjectMappers.logEntryDeserializer.readValue(response.body(), OlogLog.class);
@@ -213,7 +219,7 @@ else if (response.statusCode() >= 300) {
 
     @Override
     public LogEntry reply(LogEntry log, LogEntry inReplyTo) throws LogbookException {
-        return save(log, inReplyTo);
+        return save(log, inReplyTo, HttpMethod.PUT);
     }
 
     /**
@@ -305,32 +311,21 @@ public String getServiceUrl() {
     }
 
     /**
-     * Updates an existing {@link LogEntry}. Note that unlike the {@link #save(LogEntry, LogEntry)} API,
-     * this does not support attachments, i.e. it does not set up a multipart request to the service.
+     * Updates an existing {@link LogEntry}.
+     * <p>NOTE:</p>
+     * The list of attachments in the {@link LogEntry} may contain additional attachments added by the user. Moreover,
+     * attachments listed in the {@link LogEntry} subject to change, but not listed in the {@link LogEntry} submitted
+     * to the service, will be removed. This makes it possible for a user to update an entry such that a
+     * potentially erroneous attachment is replaced by a correct one.
      *
      * @param logEntry - the updated log entry
      * @return The updated {@link LogEntry}
      */
     @Override
-    public LogEntry update(LogEntry logEntry) {
-
-        try {
-            HttpRequest request = HttpRequest.newBuilder()
-                    .uri(URI.create(Preferences.olog_url + OLOG_PREFIX + "/logs/" + logEntry.getId() + "?markup=commonmark"))
-                    .header("Content-Type", CONTENT_TYPE_JSON)
-                    .header("Authorization", basicAuthenticationHeader)
-                    .POST(HttpRequest.BodyPublishers.ofString(OlogObjectMappers.logEntrySerializer.writeValueAsString(logEntry)))
-                    .build();
-
-            HttpResponse<String> response = httpClient.send(request, HttpResponse.BodyHandlers.ofString());
-
-            LogEntry updated = OlogObjectMappers.logEntryDeserializer.readValue(response.body(), OlogLog.class);
-            changeHandlers.forEach(h -> h.logEntryChanged(updated));
-            return updated;
-        } catch (Exception e) {
-            LOGGER.log(Level.SEVERE, "Unable to update log entry id=" + logEntry.getId(), e);
-            return null;
-        }
+    public LogEntry update(LogEntry logEntry) throws LogbookException{
+        LogEntry updated = save(logEntry, null, HttpMethod.POST);
+        changeHandlers.forEach(h -> h.logEntryChanged(updated));
+        return updated;
     }
 
     @Override
@@ -502,6 +497,25 @@ public InputStream getAttachment(Long logId, String attachmentName) {
         }
     }
 
+    @Override
+    public InputStream getAttachment(Long logId, Attachment attachment){
+        try {
+            HttpRequest request = HttpRequest.newBuilder()
+                    .uri(URI.create(Preferences.olog_url + OLOG_PREFIX + "/attachment/" + attachment.getId()))
+                    .GET()
+                    .build();
+            HttpResponse<InputStream> response = httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream());
+            if (response.statusCode() >= 300) {
+                LOGGER.log(Level.WARNING, "failed to obtain attachment: " + new String(response.body().readAllBytes()));
+                return null;
+            }
+            return response.body();
+        } catch (Exception e) {
+            LOGGER.log(Level.WARNING, "failed to obtain attachment", e);
+            return null;
+        }
+    }
+
     /**
      * @param id Unique log entry id
      * @return A {@link SearchResult} containing a list of {@link LogEntry} objects representing the
diff --git a/app/logbook/olog/ui/doc/images/ContextMenuEdit.png b/app/logbook/olog/ui/doc/images/ContextMenuEdit.png
diff --git a/app/logbook/olog/ui/doc/images/ContextMenuLogEntryTable.png b/app/logbook/olog/ui/doc/images/ContextMenuLogEntryTable.png
diff --git a/app/logbook/olog/ui/doc/index.rst b/app/logbook/olog/ui/doc/index.rst
@@ -266,6 +266,18 @@ Please consider the following limitations of the log entry group feature:
 
 .. _preferences:
 
+Updating an existing log entry
+------------------------------
+
+Existing log entries may be modified with respect to all parts of the log entry, except author and create/modify date.
+A right click on an item in the list view will bring up the context menu from which the user can select Edit, see below.
+
+**NOTE**: when editing a log entry, the attachments view will show the attachments added when the entry was created
+or modified. In this view users may add further attachments, but also remove the ones added previously. Removed attachments
+will then be accessible only when inspecting the history of the entry.
+
+.. image:: images/ContextMenuEdit.png
+
 Preferences
 -----------
 
diff --git a/app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/AttachmentsViewController.java b/app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/AttachmentsViewController.java
@@ -47,7 +47,12 @@
 import org.phoebus.framework.spi.AppResourceDescriptor;
 import org.phoebus.framework.workbench.ApplicationService;
 import org.phoebus.logbook.Attachment;
+import org.phoebus.logbook.LogClient;
 import org.phoebus.logbook.LogEntry;
+import org.phoebus.logbook.LogService;
+import org.phoebus.logbook.LogbookException;
+import org.phoebus.logbook.LogbookPreferences;
+import org.phoebus.olog.es.api.OlogHttpClient;
 import org.phoebus.ui.application.ApplicationLauncherService;
 import org.phoebus.ui.application.PhoebusApplication;
 import org.phoebus.ui.dialog.ExceptionDetailsErrorDialog;
@@ -57,6 +62,7 @@
 import java.awt.image.BufferedImage;
 import java.io.File;
 import java.io.IOException;
+import java.io.InputStream;
 import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.StandardCopyOption;
@@ -231,6 +237,12 @@ public void invalidateAttachmentList(LogEntry logEntry) {
         attachments.setAll(Collections.emptyList());
     }
 
+    /**
+     * Sets the list of {@link Attachment}s when UI is created. This list is non-empty for instance if
+     * log entry is created from OPI (in which case the attschment is automatically added to a log entry),
+     * or when editing an existing entry containing attachments.
+     * @param attachmentsList List of {@link Attachment}s
+     */
     public void setAttachments(Collection<Attachment> attachmentsList) {
         Platform.runLater(() -> {
             this.attachments.setAll(attachmentsList);
@@ -278,26 +290,33 @@ private void showPreview(Attachment attachment) {
      * @param attachment The image {@link Attachment} selected by user.
      */
     private void showImagePreview(Attachment attachment) {
-        if (attachment.getFile() != null && attachment.getFile().exists()) {
-            // Load image data off UI thread...
-            JobManager.schedule("Show image attachment", monitor -> {
-                try {
-                    BufferedImage bufferedImage = ImageIO.read(attachment.getFile());
-                    // BufferedImage may be null due to lazy loading strategy.
-                    if (bufferedImage == null) {
-                        return;
+        JobManager.schedule("Show image attachment", monitor -> {
+            try {
+                BufferedImage bufferedImage = null;
+                if (attachment.getFile() != null && attachment.getFile().exists()) { // Attachment exists on disk
+                    bufferedImage = ImageIO.read(attachment.getFile());
+                }
+                else { // Attachment must be retrieved from service
+                    LogClient logClient = LogService.getInstance().getLogFactories().get(LogbookPreferences.logbook_factory).getLogClient();
+                    InputStream inputStream = logClient.getAttachment(-1L, attachment);
+                    if (inputStream != null) {
+                        bufferedImage = ImageIO.read(inputStream);
                     }
-                    Platform.runLater(() -> {
-                        Image image = SwingFXUtils.toFXImage(bufferedImage, null);
-                        imagePreview.visibleProperty().setValue(true);
-                        imagePreview.setImage(image);
-                    });
-                } catch (IOException ex) {
-                    Logger.getLogger(AttachmentsViewController.class.getName())
-                            .log(Level.SEVERE, "Unable to load image file " + attachment.getFile().getAbsolutePath(), ex);
                 }
-            });
-        }
+                if (bufferedImage == null) {
+                    return;
+                }
+                BufferedImage _bufferedImage = bufferedImage;
+                Platform.runLater(() -> {
+                    Image image = SwingFXUtils.toFXImage(_bufferedImage, null);
+                    imagePreview.visibleProperty().setValue(true);
+                    imagePreview.setImage(image);
+                });
+            } catch (Exception ex) {
+                Logger.getLogger(AttachmentsViewController.class.getName())
+                        .log(Level.SEVERE, "Unable to load image file " + attachment.getName(), ex);
+            }
+        });
     }
 
     /**
diff --git a/app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/OlogAttributeProvider.java b/app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/OlogAttributeProvider.java
@@ -18,17 +18,15 @@
 
 package org.phoebus.logbook.olog.ui;
 
+import org.apache.commons.io.FilenameUtils;
 import org.commonmark.ext.gfm.tables.TableBlock;
 import org.commonmark.renderer.html.AttributeProvider;
-
-import java.util.Map;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import org.phoebus.logbook.Attachment;
-import java.util.List;
-import org.apache.commons.io.FilenameUtils;
 import org.phoebus.olog.es.api.OlogHttpClient;
 
+import java.util.List;
+import java.util.Map;
+
 /**
  * An {@link AttributeProvider} used to style elements of a log entry. Other types of
  * attribute processing may be added.
@@ -39,18 +37,19 @@ public class OlogAttributeProvider implements AttributeProvider {
     private boolean preview = false;
     private List<Attachment> attachments;
 
-    public OlogAttributeProvider(String serviceUrl){
+    public OlogAttributeProvider(String serviceUrl) {
         this.serviceUrl = serviceUrl;
     }
 
     /**
      * This is constructor for HTML preview feature
-     * @param serviceUrl Olog service url.
-     * @param preview A boolean flag set true for HTML preview feature parsing.
+     *
+     * @param serviceUrl  Olog service url.
+     * @param preview     A boolean flag set true for HTML preview feature parsing.
      * @param attachments A list of current attachments which can be parsed to
-     * find attachment file path from attachment id.
+     *                    find attachment file path from attachment id.
      */
-    public OlogAttributeProvider(String serviceUrl, boolean preview, List<Attachment> attachments){
+    public OlogAttributeProvider(String serviceUrl, boolean preview, List<Attachment> attachments) {
         this.serviceUrl = serviceUrl;
         this.preview = preview;
         this.attachments = attachments;
@@ -80,12 +79,17 @@ public void setAttributes(org.commonmark.node.Node node, String s, Map<String, S
                 src = serviceUrl + OlogHttpClient.OLOG_PREFIX + "/" + src;
             }
             // If preview flag is true, the image url 'attachment/attachment_id'
-            // has to be converted to 'file://attachment_path'
+            // has to be converted to 'file://attachment_path' if on disk.
+            // If not on disk (log entry edit case), create a regular URL to retrieve attachment.
             if (src.startsWith("attachment") && this.preview) {
-                String attachmentId = src.substring(11, src.length());
-                for (Attachment attachment: attachments) {
+                String attachmentId = src.substring(11);
+                for (Attachment attachment : attachments) {
                     if (attachment.getId().equals(attachmentId)) {
-                        src = "file://" + FilenameUtils.separatorsToUnix(attachment.getFile().getAbsolutePath());
+                        if (attachment.getFile() != null) {
+                            src = "file://" + FilenameUtils.separatorsToUnix(attachment.getFile().getAbsolutePath());
+                        } else {
+                            src = serviceUrl + OlogHttpClient.OLOG_PREFIX + "/attachment/" + attachmentId;
+                        }
                     }
                 }
             }
diff --git a/app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/write/AttachmentsEditorController.java b/app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/write/AttachmentsEditorController.java
@@ -138,14 +138,13 @@ public void initialize() {
             attachments.forEach(a -> {
                 if (a.getFile() != null) {
                     attachedFilesSize += getFileSize(a.getFile());
+                    filesToDeleteAfterSubmit.add(a.getFile());
                 }
             });
         }
 
         attachmentsViewController.setAttachments(attachments);
 
-        filesToDeleteAfterSubmit.addAll(attachments.stream().map(Attachment::getFile).toList());
-
         removeButton.setGraphic(ImageCache.getImageView(ImageCache.class, "/icons/delete.png"));
         removeButton.disableProperty().bind(Bindings.isEmpty(attachmentsViewController.getSelectedAttachments()));
 
diff --git a/app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/write/LogEntryEditorController.java b/app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/write/LogEntryEditorController.java
@@ -309,7 +309,6 @@ public void initialize() {
         templateControls.visibleProperty().setValue(editMode.equals(EditMode.NEW_LOG_ENTRY));
 
         attachmentsPane.managedProperty().bind(attachmentsPane.visibleProperty());
-        attachmentsPane.visibleProperty().setValue(editMode.equals(EditMode.NEW_LOG_ENTRY));
 
         // This could be configured in the fxml, but then these UI components would not be visible
         // in Scene Builder.
@@ -734,13 +733,11 @@ public void submit() {
             ologLog.setLevel(selectedLevelProperty.get());
             ologLog.setLogbooks(getSelectedLogbooks());
             ologLog.setTags(getSelectedTags());
-            if (editMode.equals(EditMode.NEW_LOG_ENTRY)) {
-                ologLog.setAttachments(attachmentsEditorController.getAttachments());
-            }
-            else{
+            ologLog.setAttachments(attachmentsEditorController.getAttachments());
+            ologLog.setProperties(logPropertiesEditorController.getProperties());
+            if (editMode.equals(EditMode.UPDATE_LOG_ENTRY)) {
                 ologLog.setId(logEntry.getId());
             }
-            ologLog.setProperties(logPropertiesEditorController.getProperties());
 
             LogClient logClient =
                     logFactory.getLogClient(new SimpleAuthenticationToken(usernameProperty.get(), passwordProperty.get()));

Original file line number	Diff line number	Diff line change
`@@ -138,14 +138,13 @@ public void initialize() {`
`138`	`138`	`attachments.forEach(a -> {`
`139`	`139`	`if (a.getFile() != null) {`
`140`	`140`	`attachedFilesSize += getFileSize(a.getFile());`
	`141`	`+ filesToDeleteAfterSubmit.add(a.getFile());`
`141`	`142`	`}`
`142`	`143`	`});`
`143`	`144`	`}`
`144`	`145`
`145`	`146`	`attachmentsViewController.setAttachments(attachments);`
`146`	`147`
`147`		`- filesToDeleteAfterSubmit.addAll(attachments.stream().map(Attachment::getFile).toList());`
`148`		`-`
`149`	`148`	`removeButton.setGraphic(ImageCache.getImageView(ImageCache.class, "/icons/delete.png"));`
`150`	`149`	`removeButton.disableProperty().bind(Bindings.isEmpty(attachmentsViewController.getSelectedAttachments()));`
`151`	`150`