Fix collection failure if entry fails. #1093

tech4him1 · 2018-02-08T21:45:51Z

- Summary

Before, if any entry in a collection failed to load, the entire collection would be empty. This PR hides only the files that actually fail, instead of the entire collection.

Partial fix for #1092 and #929.

- Test plan

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

verythorough · 2018-02-08T21:47:38Z

Deploy preview for netlify-cms-www ready!

Built with commit 2b01649

https://deploy-preview-1093--netlify-cms-www.netlify.com

verythorough · 2018-02-08T21:48:10Z

Deploy preview for cms-demo ready!

Built with commit 2b01649

https://deploy-preview-1093--cms-demo.netlify.com

tech4him1 · 2018-02-08T21:56:38Z

@Benaiah Do you see any problems with this?

I'm thinking we should eventually do some sort of toast notification if a file fails to load, or something like that.

Benaiah · 2018-02-08T22:18:49Z

src/backends/backend.js

@@ -133,6 +133,7 @@ class Backend {
    const extension = selectFolderEntryExtension(collection);
    const collectionFilter = collection.get('filter');
    return listMethod.call(this.implementation, collection, extension)
+      .then(loadedEntries => loadedEntries.filter(loadedEntry => !loadedEntry.error))


Filtering out entries that failed to load seems like an implementation detail of the backend to me, and signalling by resolving to an object with an error property is pretty confusing, especially since it requires coordinated keys in two different files. I would suggest one of two refactors:

This .then(x => x.filter(...)) line could be moved into src/backends/github/implementation.js, right after the return Promise.all(...) line. (This is the refactor that seems best to me.)

You could do all the filtering here by dropping the Promise.all in fetchFiles and returning an array of Promises instead, then map .catch over them here, instead of in src/backends/github/implementation.js. (I don't like this refactoring as much - catching these errors seems like a detail that should be handled by the backends.)

Agreed, I'll move it.

Benaiah · 2018-02-08T22:20:42Z

@tech4him1 doing a notification eventually sounds like a good idea. I think we should have a way for backends to signal errors that can be stored in redux to trigger notifications/correction logic/etc. That would go well with the plan to have backends store their state in the redux store.

tech4him1 · 2018-02-08T23:01:25Z

I think we should have a way for backends to signal errors that can be stored in redux to trigger notifications/correction logic/etc.

@Benaiah How would you see this signalling happen in this fetching scenario? Would there be a function passed down to the backend, or would the backend pass an error object back up?

Benaiah · 2018-02-09T00:05:08Z

@Benaiah How would you see this signalling happen in this fetching scenario? Would there be a function passed down to the backend, or would the backend pass an error object back up?

@tech4him1 my first instinct is to do this by passing a function into the backend. Let me know what you think about this design:

Errors that are critical enough to prevent the application from moving forward with its current task should remain as traditional exceptions or promise rejections.
Errors that are not critical, but could require action (refetching a post, regenerating metadata when it's corrupt or unreadable, etc.) should be passed into the application so the route to proceed can be decided by a higher level of the application (say, a modal that asks the user whether they'd like to try to reload entries that didn't appear, or just a notification that lets a user know a specific post didn't load due to an error). These should be stored in the redux store, and the backend should be given a function that lets it register information about errors and how to resolve them.
Transient errors that are either intentional or can be automatically corrected by the backend should be caught within the backend and remain invisible to the higher-level code.

tech4him1 · 2018-02-09T01:44:41Z

@Benaiah On the second one ("optional errors"), the backend would return successfully after it registered the error. Then the higher-level component can decide to re-call the backend, but that would be a separate call and action chain. Right? I don't see any inherent problems with that outline.

Benaiah · 2018-02-09T04:18:33Z

@tech4him1 yeah, that's my thought. The backend may need to be involved in deciding on a resolution for the error, but we can do that by passing that info back up the chain.

erquhart

LGTM

tech4him1 · 2018-02-28T23:40:30Z

@Benaiah @erquhart An interesting idea @peduarte brought up in #1092 was, instead of showing an error message, show the broken entry as "disabled" (maybe greyed out) in the list. What's your opinion on that?

Benaiah · 2018-02-28T23:52:57Z

@tech4him1 I don't care for greyed-out UI elements - they seem non-interactive and they don't explain why they're greyed out. Maybe if we displayed it similarly to a validation failure, with a small attached message in red explaining that the entry failed to load?

Additionally, an entry that fails to load won't be able to show up as a standard list entry, since we don't know any of its metadata, such as its title (it would have needed to load for us to know that).

There's also the issue of nontechnical users not understanding why entries are present in the entry list but cannot be loaded or interacted with.

IMO an error message is the simpler option, since I don't see easy solutions to those concerns, but I might be missing something.

tech4him1 · 2018-03-01T00:17:07Z

@Benaiah The only reason I don't like a standard toast error message is its timeout -- it doesn't seem to work that well for stuff like this, where multiple entries could possibly have failed.

Benaiah · 2018-03-01T00:32:22Z

@tech4him1 what about something like "Some entries failed to load, please contact your CMS administrator or see the console for details`? I don't think the actual error messages are going to be very actionable by non-technical users anyway - I'd just want to communicate to them that there's a reason that some of the posts may be missing. I'm guessing that anyone capable of fixing their CMS deployment based on the error message is likely to already know how to open the browser console.

tech4him1 · 2018-03-01T00:50:55Z

@Benaiah I don't see any problem with something like that, as most cases aren't going to be actionable by the user. There are some exceptions such as creating a new file that doesn't exist (for files collections), but that may be better done in a different way.

tech4him1 · 2018-03-05T21:49:51Z

@erquhart Do you want to add a toast/notification to this before merge, or do that later?

erquhart · 2018-03-05T22:45:44Z

That's an improvement, I want to start merging stuff fast. Let's get this in and worry about improving in a separate PR.

tech4him1 · 2018-03-05T23:16:32Z

src/backends/github/implementation.js

          sem.leave();
-          reject(err);
+          console.error(`failed to load file from GitHub: ${file.path}`);


@erquhart @Benaiah I added a console error for now, we can add a toast notification or other GUI action later.

tech4him1 · 2018-03-05T23:20:00Z

Great, let's merge.

tech4him1 added 4 commits February 8, 2018 14:41

Update backend.js

563ddd3

Update implementation.js

d3afcc3

Update backend.js

919b5e4

Update implementation.js

ca87013

Benaiah reviewed Feb 8, 2018

View reviewed changes

tech4him1 added 2 commits February 8, 2018 20:03

Update backend.js

d61e51b

Update implementation.js

501c72c

erquhart approved these changes Feb 21, 2018

View reviewed changes

tech4him1 self-assigned this Feb 28, 2018

Log console error if file fails to load.

2b01649

tech4him1 commented Mar 5, 2018

View reviewed changes

tech4him1 changed the title ~~WIP: Fix collection failure if entry fails.~~ Fix collection failure if entry fails. Mar 5, 2018

tech4him1 added kind: bug kind: discussion area: api labels Mar 5, 2018

tech4him1 added kind: enhancement area: extensions/backends area: error-handling area: file-handling labels Mar 5, 2018

tech4him1 merged commit 95b6d8a into master Mar 5, 2018

tech4him1 deleted the fixEntryFail branch March 5, 2018 23:31

This was referenced Mar 5, 2018

Collections fail to load if any of the Github request is above 1mb #1092

Closed

Confusing behavior when you add collection config without creating entry file #929

Closed

tech4him1 mentioned this pull request Jun 5, 2018

GitLab backend built with cursor API #1343

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix collection failure if entry fails. #1093

Fix collection failure if entry fails. #1093

tech4him1 commented Feb 8, 2018 •

edited

Loading

verythorough commented Feb 8, 2018 •

edited

Loading

verythorough commented Feb 8, 2018 •

edited

Loading

tech4him1 commented Feb 8, 2018 •

edited

Loading

Benaiah Feb 8, 2018

tech4him1 Feb 8, 2018

Benaiah commented Feb 8, 2018 •

edited

Loading

tech4him1 commented Feb 8, 2018

Benaiah commented Feb 9, 2018 •

edited

Loading

tech4him1 commented Feb 9, 2018 •

edited

Loading

Benaiah commented Feb 9, 2018

erquhart left a comment

tech4him1 commented Feb 28, 2018

Benaiah commented Feb 28, 2018 •

edited

Loading

tech4him1 commented Mar 1, 2018

Benaiah commented Mar 1, 2018 •

edited

Loading

tech4him1 commented Mar 1, 2018

tech4him1 commented Mar 5, 2018

erquhart commented Mar 5, 2018

tech4him1 Mar 5, 2018

tech4him1 commented Mar 5, 2018

Fix collection failure if entry fails. #1093

Fix collection failure if entry fails. #1093

Conversation

tech4him1 commented Feb 8, 2018 • edited Loading

verythorough commented Feb 8, 2018 • edited Loading

verythorough commented Feb 8, 2018 • edited Loading

tech4him1 commented Feb 8, 2018 • edited Loading

Benaiah Feb 8, 2018

Choose a reason for hiding this comment

tech4him1 Feb 8, 2018

Choose a reason for hiding this comment

Benaiah commented Feb 8, 2018 • edited Loading

tech4him1 commented Feb 8, 2018

Benaiah commented Feb 9, 2018 • edited Loading

tech4him1 commented Feb 9, 2018 • edited Loading

Benaiah commented Feb 9, 2018

erquhart left a comment

Choose a reason for hiding this comment

tech4him1 commented Feb 28, 2018

Benaiah commented Feb 28, 2018 • edited Loading

tech4him1 commented Mar 1, 2018

Benaiah commented Mar 1, 2018 • edited Loading

tech4him1 commented Mar 1, 2018

tech4him1 commented Mar 5, 2018

erquhart commented Mar 5, 2018

tech4him1 Mar 5, 2018

Choose a reason for hiding this comment

tech4him1 commented Mar 5, 2018

tech4him1 commented Feb 8, 2018 •

edited

Loading

verythorough commented Feb 8, 2018 •

edited

Loading

verythorough commented Feb 8, 2018 •

edited

Loading

tech4him1 commented Feb 8, 2018 •

edited

Loading

Benaiah commented Feb 8, 2018 •

edited

Loading

Benaiah commented Feb 9, 2018 •

edited

Loading

tech4him1 commented Feb 9, 2018 •

edited

Loading

Benaiah commented Feb 28, 2018 •

edited

Loading

Benaiah commented Mar 1, 2018 •

edited

Loading