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

Refactors creating and previewing deliveries #25

Merged
merged 34 commits into from
Aug 31, 2018
Merged

Conversation

dleehr
Copy link
Member

@dleehr dleehr commented Aug 30, 2018

  • Removes queryParam binding from the URLs in creating deliveries
  • Changes deliveries.new route to create a delivery as its model, and sub-routes receive it as delivery
  • Moves delivery previewing to a component
  • Updates project and user picker to send actions based on selectedItems array
  • Refactors delivery builder routes to be more declarative and uses a base class for error handling and computed properties
  • Fixes issue in development mode where logging in would not redirect from login page

dleehr added 30 commits August 23, 2018 15:35
- Working in controllers, have reduced select-project and select-recipient
- Have not touched template code yet
- Need to implement project permission testing and filtering out bad users
- user list sorting was causing the userSelectionChanged event to fire because it updated the list
- transitionToRoute params building is ugly. May want to bind in queryParams in processBack/processNext
- Still have some missing functionality'
- Removes queryParams
- Replaces actions and transitionToRoute logic with link-to in template
- Removes custom setters/getters based on ids in favor of aliases to the model fields
- Uses the forceReload flag on the model hook to prevent us from see a partial list
…selection

- Updates project listing component to observe the selectedItems array and call the action when that changes (rather than leak implementation details of the underlying ember-models-table
- Adds action hooks (willPerform, didPerform, didFail) in base.js
…fferent

- just moved things into the service and set a property on the service
This controller passes the delivery as a relationship from transfer
that may not have been loaded yet, so we need to check if the model is
loaded  (and resolve) before calling preview()
- restores sorting on user list, since we're not handling displayDataChanged anymore
- Ember link-to adds an 'active' class if the route matches. Apparently 'deliveries' matches 'deliveries.new.select-project'?
- Moves logic for setting the fromUser here, since it's concerned with its primary model
- Makes the controller available for injection to the tests
- Instead following ember's recommendation to set in route.setupController
- Since there are multiple routes, done with a Mixin
@dleehr dleehr requested a review from johnbradley August 30, 2018 20:22
if(delivery.get('isLoaded')) {
delivery = Ember.RSVP.resolve(delivery);
}
delivery.then((loadedDelivery) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Had to read this a couple times to figure out what is going on:
If delivery is not a promise make it one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the comment around this. I still don't like how this works but have been digging in to understand why it's not working.

The underlying issue is that when I create a{{delivery-preview}} component in resend-confirm.hbs, the delivery object I provide is model.delivery, where model is a duke-ds-project-transfer.

When delivery-preivew.js accesses that delivery, it's a DS.PromiseObject. DS.PromiseObjects do not a have a preview() method, so the promise needs to be resolved to a "real" delivery before calling preview(). If I call delivery.get('preview')(), it does seem find the function, but then it's not bound to the right this.

Using {{delivery-preview}} from deliveries/new/confirm.hbs doesn't have this problem. That delivery object is the model for the route, so by the time it gets to the template, it's fully loaded and resolved.

Perhaps the best (and conventional) solution is for the deliveries/show/confirm-resend route to return the delivery as its model. The transfer can be accessed as a model from deliveries/show

Copy link
Contributor

@johnbradley johnbradley left a comment

Choose a reason for hiding this comment

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

LGTM.
Only concern is if user clicks refresh button on select-recipient or enter-message the preview page shows an error. Perhaps you could break this out into an issue.


/* May override */
disableNext: false,
working: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Is working used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not anymore. I experimented using it to show a loading indicator when fetching project permissions, but didn't have an obvious place to put that indicator.

@johnbradley
Copy link
Contributor

Confirm delivery/preview page error if user clicks refresh:
screen shot 2018-08-31 at 9 01 39 am

@dleehr
Copy link
Member Author

dleehr commented Aug 31, 2018

Fixes #19

@dleehr dleehr merged commit ecd418d into master Aug 31, 2018
@dleehr dleehr deleted the refactor-preview branch August 31, 2018 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants