Skip to content
This repository has been archived by the owner on Apr 18, 2018. It is now read-only.

Rescue failed gallery upload #111

Merged
merged 3 commits into from
Jan 25, 2015
Merged

Rescue failed gallery upload #111

merged 3 commits into from
Jan 25, 2015

Conversation

rzane
Copy link
Contributor

@rzane rzane commented Jan 21, 2015

When a file failed to upload to a gallery, the loading icon would just sit there spinning. The error wasn't being caught. The imageUploadFailed function was added because the error behavior was used in multiple places. The problem can be replicated by attempting to upload a file with a strange extension.

After handling the errors, the refresh button wasn't being hidden after being clicked. Every time it was clicked, it would refetch the images without clearing the images that were already there.

Also, as far as I know, there's no reason to use class_eval inside the included block when using ActiveSupport::Concern. The included block is for class level macros, like has_one or attr_accessor. No need to put instance methods in the included block.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 94.82% when pulling 869968c on rzane:catch-gallery-upload-errors into 59105dd on volmer:master.

@volmer
Copy link
Owner

volmer commented Jan 22, 2015

Good catch! This is a very good improvement. And you're totally right about ActiveSupport::Concern too. Can I ask you to just add a Cucumber scenario to cover the error handling?

Thanks a million times for this PR! 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.25%) to 95.08% when pulling 60334cf on rzane:catch-gallery-upload-errors into 59105dd on volmer:master.

@rzane
Copy link
Contributor Author

rzane commented Jan 24, 2015

Hey @volmer, any reason this hasn't been merged yet?

@volmer
Copy link
Owner

volmer commented Jan 24, 2015

Amazing work, @rzane. Sorry for the delay, I wasn't aware you've updated it. Could you please fix the conflicts so I can proceed with the merge?

@rzane
Copy link
Contributor Author

rzane commented Jan 24, 2015

Absolutely. I'll rebase asap.

Reset the gallery after pressing the refresh button

No need for class_eval when extending ActiveSupport::Concern
Remove the update route
Add upload spec for invalid w/ url
@coveralls
Copy link

Coverage Status

Coverage increased (+0.26%) to 95.09% when pulling 73491c5 on rzane:catch-gallery-upload-errors into dea651d on volmer:master.

@rzane
Copy link
Contributor Author

rzane commented Jan 24, 2015

I can squash my commits if you'd prefer. This should merge cleanly now.

In my most recent commit, I removed update from the routes, since it wasn't in use. I also added a config option for Bootsy.base_controller. Setting it to ApplicationController would allow users to control authentication more easily.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.26%) to 95.09% when pulling de4a9c8 on rzane:catch-gallery-upload-errors into dea651d on volmer:master.

volmer added a commit that referenced this pull request Jan 25, 2015
@volmer volmer merged commit 9ab3789 into volmer:master Jan 25, 2015
@volmer
Copy link
Owner

volmer commented Jan 25, 2015

Thank you for the hard work! ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants