Provide better feedback in the case where a capture cannot be taken (for example, if there was an error retrieving content) by displaying an error in the 'message' field in the dashboard.

For reference, the analogous change in the Wordpress version of the plugin is: https://plugins.trac.wordpress.org/changeset?sfp_email=&sfph_mail=&repon...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jlicht created an issue. See original summary.

skbly7’s picture

Steps followed:

- Update of "libraries" directory with latest "amber_common" sync.
- The size column has been loaded with responsibility to display errors instead of defining new column:
- Reason: Already too many columns, thus message column was felt unnecessary.

Feature Addition/Modification:

- Removal of Up/Down value in case of "size too large" errors to avoid confusion.
- Addition of error message in case of "size too large" in size column.

skbly7’s picture

Version: 7.x-1.x-dev » 7.x-1.0
Assigned: Unassigned » skbly7
Status: Active » Needs work
FileSize
7.43 KB

Steps followed:

- Update of "libraries" directory with latest "amber_common" sync.
- The size column has been loaded with responsibility to display errors instead of defining new column:
- Reason: Already too many columns, thus message column was felt unnecessary.

Feature Addition/Modification:

- Removal of Up/Down value in case of "size too large" errors to avoid confusion.
- Addition of error message in case of "size too large" in size column.

Feature related file changed are: "amber.pages.inc" and "amber.batch.inc".

Patch for both 7.x and 7.0 branch is attached.

skbly7’s picture

Patch included:

- libraries folder latest synced.
- feature implementation
- display of message in "size" column (reason: there were already too many columns, so instead of adding new "message" column, size was overloaded with message's functionality.
- removal of up/down message in case of "size too large" error.

File changes description:

- All "libraries" folder change are because of sync.
- The other two files to add functionalities.

jlicht’s picture

Version: 7.x-1.0 » 7.x-1.x-dev
Status: Needs review » Needs work

Regarding

+ * Return the value for status column based on status and last checked
+ */
+function _amber_status_column_value($status, $last_checked = "2 ") {
+  $value = $status ? 'Up' : 'Down';
+  if ($last_checked != NULL && empty($last_checked)) {
+    $value = "";
+  }
+  return $value;
+}

Two comments:
* I don't think we need to provide a default value for $last_checked.
* The logic for checking $last_checked behaves differently if the value is NULL and if it's an empty string - this is confusing.

I think this function can be similar to the Wordpress one:

+ * Return the value for status column based on status and last checked
*/
function _amber_status_column_value($status, $last_checked) {
 if (empty($last_checked)) {
   return "";
 } else {
   return $status ? 'Up' : 'Down';
}

If you agree, please preroll the patch and resubmit.

skbly7’s picture

Status: Needs work » Needs review
FileSize
7.4 KB

It has been mistype from my side on "_amber_status_column_value" function, I intended to have NULL as default value instead of "2 ", but ended up submitting patch of wrong function. I am extremely sorry for it, and will take care for future patches.

Also, I agree on both your comments and have removed default value from the last_checked parameter. Resubmitting the patch with updated function.

jlicht’s picture

Status: Needs review » Needs work

The change to the _amber_status_column_value() function looks good.

One more change before applying this patch - PHP close tags have been added to a few of the files. This is not desired - see https://www.drupal.org/coding-standards#phptags

jlicht’s picture

Assigned: skbly7 » jlicht

  • jlicht committed 0bbce48 on 7.x-1.x
    Issue #2717693 by skbly7, jlicht: Display a message in the dashboard if...
jlicht’s picture

Status: Needs work » Fixed

Committed a change that builds upon the previous patches, but adds some more features:

* Adds an extra column ('message') to the amber_cache table which allows us to track errors associated with the caching process for a specific backend, rather than for the checking process, which is backend-agnostic. For example, if you are caching to Perma and your local disk, you might receive an error from Perma, but still be able to save locally. This column allows us to keep the error message associated with the backend, and display appropriately on the frontend

* Updated the AmberFetcher, PermaFetcher, and InternetArchiveFetcher to provide more information in case of errors (e.g. url, provider id) rather than just an error message string - which allows us to save an error message and associate it with a specific cache attempt with a specific backend

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.