Skip to content

Redirect user to projects.index from projects.donate, if donation already exist - #803

Merged
joshsmith merged 1 commit into
developfrom
798-prevent-loading-of-donate-route-if-already-subscriber
Dec 1, 2016
Merged

Redirect user to projects.index from projects.donate, if donation already exist#803
joshsmith merged 1 commit into
developfrom
798-prevent-loading-of-donate-route-if-already-subscriber

Conversation

@begedin

@begedin begedin commented Nov 29, 2016

Copy link
Copy Markdown
Contributor

What's in this PR?

Changes the flow on the donate page to redirect to main project route if donation already exists.

Since it reuses the same logic as the project.index route to find the user's subscription for the current project, I moved that logic into a service.

I also added a flash message, but it doesn't seem to work for some reason. Flash messages do not seem to be broken, but I couldn't find a problem. Any pointers are welcome.

References

Fixes #798

Comment thread app/routes/project/donate.js Outdated

afterModel({ project, subscription }) {
if (subscription) {
get(this, 'flashMessages').success('We are grateful for eagerness, but you are supporting us already.');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should buff this language:

You're already supporting this project.

@joshsmith

Copy link
Copy Markdown
Contributor

@begedin I think flash messages aren't showing maybe because we clear them at the router transition level. I'm not sure if we are doing that, but it seems non-obvious means that it was a fix for something that went undocumented and is now not working as we would expect. Too hacky.

@joshsmith joshsmith modified the milestones: Donations, Improve Donations Nov 30, 2016
@begedin
begedin force-pushed the 798-prevent-loading-of-donate-route-if-already-subscriber branch from 7196dbf to fe8ad69 Compare November 30, 2016 15:07
@begedin

begedin commented Nov 30, 2016

Copy link
Copy Markdown
Contributor Author

@joshsmith We actually do clear messages on transition, which means two things.

  1. They will clear when we switch routes.
  2. In hooks such as afterModel, even if we stay on the same route, didTransition is called after, so it will clear any message added through such a hook.

My guess is, we are clearing messages because of most of the flashes we actually display are "sticky", meaning they never go away until the user dismisses them via click. Since all of these cases where there is a sticky also specify a timeout, my guess would be, we were adding them incorrectly to begin with.

My suggestion is, we

  • remove the sticky from all of them
  • remove the explicit timeout and use a default timeout
  • use helpers (.success(message), .warning(message), etc.) instead of the default add(optionsHash) to keep things simpler.
  • remove the clearing on didTransition

@begedin
begedin force-pushed the 798-prevent-loading-of-donate-route-if-already-subscriber branch from 5282c60 to 0492c4a Compare December 1, 2016 09:18
@begedin
begedin force-pushed the 798-prevent-loading-of-donate-route-if-already-subscriber branch from 0492c4a to 3935cc5 Compare December 1, 2016 12:35
@begedin

begedin commented Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

@joshsmith I rewrote the flash messages and added some convenience test helpers for them, so we don't have to stub the service anymore.

From now on, every time we display a message, we explicitly clear the message queue first. It didn't look like we need to not clear it in any place, but it's easily switchable now.

@begedin begedin self-assigned this Dec 1, 2016

@alexgraffeocohen alexgraffeocohen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just before you merge, you might as well fix this Mirage model as well, something @joshsmith discovered.

screen shot 2016-12-01 at 8 48 25 am

@joshsmith

Copy link
Copy Markdown
Contributor

@alexwilkinson addressed that change.

…ady exists

Rework flash messages do they don't clear on route transition

Write test helpers for flash messages, to avoid stubbing the service
@joshsmith
joshsmith force-pushed the 798-prevent-loading-of-donate-route-if-already-subscriber branch from af4e85c to c22a6dd Compare December 1, 2016 17:14
@joshsmith
joshsmith merged commit 38c72d0 into develop Dec 1, 2016
@joshsmith
joshsmith deleted the 798-prevent-loading-of-donate-route-if-already-subscriber branch December 1, 2016 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants