Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: No re-auth modal when saving editor with invalid session #6048

Closed
kevinansfield opened this issue Nov 5, 2015 · 2 comments · Fixed by #6061
Closed

Regression: No re-auth modal when saving editor with invalid session #6048

kevinansfield opened this issue Nov 5, 2015 · 2 comments · Fixed by #6061
Assignees
Labels
affects:admin Anything relating to Ghost Admin bug [triage] something behaving unexpectedly P2 - High [triage] High priority for immediate patch release

Comments

@kevinansfield
Copy link
Contributor

Steps to reproduce:

  1. Create/edit a post and ensure it's saved
  2. In web inspector find your ghost:sessionin the Local Storage section under the Resources tab and edit your access_token string
  3. Change the post body and click save

Expected result: You are shown the re-authenticate modal from which you're are able to sign-in and continue

Actual result: You are shown an error alert: "Saving failed. The server returned an error (Server was not available)."

@kevinansfield kevinansfield added bug [triage] something behaving unexpectedly affects:admin Anything relating to Ghost Admin P2 - High [triage] High priority for immediate patch release in progress labels Nov 5, 2015
@kevinansfield kevinansfield self-assigned this Nov 5, 2015
@kevinansfield kevinansfield changed the title Bug: Not shown re-auth modal when attempting to save editor with invalid session Regression: No re-auth modal when saving editor with invalid session Nov 5, 2015
@kevinansfield
Copy link
Contributor Author

I have a failing test in place for this. It appears that ember-simple-auth no longer raises the authorizationFailed action so we need to find the correct way to hook into this state with ESA 1.0.

@kevinansfield
Copy link
Contributor Author

I think the solution to this is going to be further reaching than I first thought. The core problem is this:

  • In ValidationEngine we override .save to handle validation (which is fine) but we also capture any errors raised and use getRequestErrorMessage (from utils/ajax.js) to replace the original error object with a string.
  • Anywhere we use the .save().catch() pattern to do custom error handling we now either have an array of validation errors or a plain string - we've lost the ability to react according to the type of error we receive at the very edge of our application code
  • .save().catch() has become a common pattern where we typically ignore the validation errors (they get handled by our inline validation) and then fire off an alert with the string returned from getRequestErrorMessage, and finally return nothing - we've now lost the ability to use Ember's error action in routes. The error action is useful as it starts at the furthest edge route and bubbles up to the application route, allowing a spectrum of fine-grained to high-level responses to an error.

Taking into consideration we eventually want to refactor ValidationEngine and update our API to return DS.Errors compatible error responses and possibly i18n keys I plan implement something like the following:

  1. Remove the .catch In ValidationEngine.save(), this would result in the following behaviour:
    • reject with undefined if we have validation errors (current behaviour)
    • reject with with the original error object if we hit a server/other error (new behaviour)
  2. Review everywhere we use the .save().catch() pattern to ensure that we are only handling the exceptional cases we care about, always return/throw the original error in a way that Ember will pick up
  3. Use the error actions on leaf-routes where appropriate, eg. In the editor to show the re-auth modal
  4. Add an error action to the application route that uses getRequestErrorMessage to show a suitable alert
  5. Move logic from notifications.showAPIError into getRequestErrorMessage - this leaves the notifications service dealing purely with handling basic alerts/notifications

kevinansfield added a commit to kevinansfield/Ghost that referenced this issue Nov 6, 2015
kevinansfield added a commit to kevinansfield/Ghost that referenced this issue Nov 6, 2015
kevinansfield added a commit to kevinansfield/Ghost that referenced this issue Nov 12, 2015
refs TryGhost#6039, closes TryGhost#6047, closes TryGhost#6048

- delete old/unused fixtures file
- add failing tests for TryGhost#6047 & TryGhost#6048
- redirect to sign-in if we get a 401 when making an API request
- fix incorrect `this.notifications` call in tag controller
- raise `authorizationFailed` action in application route's `sessionInvalidated` hook so that it can be handled by leaf routes (fixes re-auth modal display)
- close "saving failed" alert when successfully re-authenticated
- adds a "window-proxy" util so that we can override `window.*` operations in tests
- fix `gh-selectize` attempting to register event handlers when the component has already been destroyed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:admin Anything relating to Ghost Admin bug [triage] something behaving unexpectedly P2 - High [triage] High priority for immediate patch release
Projects
None yet
1 participant