Allow users to create deliveries #21

johnbradley · 2018-07-26T14:57:52Z

Adds changes to allow a user to select a project, recipient and enter a message to be included in the email.
High Level Changes

New Delivery button to the Outgoing deliveries
deliveries/new/select-project route where user selects a project to deliver
new/select-recipient?project_id=<id> route where user selects recipient ~~and share users~~
deliveries/new/enter-user-message?project_id=<id>&to_user_id=<id>... route to enter a message and send the delivery

Future changes

make use of DukeDS affiliates for selecting recipient and share users

Needs Duke-GCB/D4S2#169

Fixes #6

Adds deliveries/new/select-project route deliveries/new/select-recipient route deliveries/new/enter-user-message route Passses IDs as queryParams between routes TODO: replace underscore query params with camel case filter projects to those we can actually deliver filter recipient to not include current user cleanup select recipient screen - still confusing

…ew-deliveries

Adds checkboxes for selecting projects and users. Filters users to exclude current user.

also fixes failing new tests for controllers

application now contains logic for currentDukeDsUser so this is no longer needed.

adds tests for new routes

Th computed field shareUsers returns a javascript array of promises so we can't just call `then` on the returned array.

Switch to match existing pattern.

dleehr

Code looks good, only had a couple in-line observations. I wanted to see how this worked, so most of the feedback below is about the select-recipients page:

A label/heading for the primary recipient should be more prominent (maybe an h4?), and indicate that this is required.
"Select additional users to share project." - The meaning of this text is not clear. This text should describe that this area is for selecting additional users that will get access to the data but not ownership, and that this is optional.
While the table of users is sortable, it doesn't come up in any discernable order. Perhaps sort by last name initially? (That could be done in the JS)
There are some empty rows in my listing - We should probably filter these out in the UI. I can't imagine any good things can come from delivering to these users :)
The name and email filters are case-sensitive, which I didn't expect or discover right away. At first I thought it was broken. I used a filterFunction for doing case-insensitive filtering on project statuses, and that may help here.
It's possible to select the primary user as one of the additional "share with" users, which doesn't make sense.
The more I think about this, the more I think the "picking additional users" part should be a separate step. It makes it easy to filter out the primary user and lets the user focus or skip that step more easily.

dleehr · 2018-08-06T15:50:13Z

app/controllers/deliveries/new/enter-user-message.js

+import Ember from 'ember';
+
+export default Ember.Controller.extend({
+  queryParams: ['project_id', 'to_user_id', 'share_user_ids'],


Is there a reason for using snake_case for some variables and parameters?

I was trying to match the underlying API but it does look weird. Will fix.

dleehr · 2018-08-06T15:51:20Z

app/controllers/deliveries/new/enter-user-message.js

+  }),
+  shareUsers: Ember.computed('share_user_ids', function () {
+    const store = this.get('store');
+    const shareUserIds = this.get('share_user_ids');


I notice that the shareUserIds are passed around as a comma-separated string. Do arrays not work as queryParam variables?

In my experience it automatically comma separates (%2C) array elements, but this seems rather unexpected and not deserialized on the other end.
It sounds like query parameters serialization is an ongoing issue in ember: emberjs/ember.js#14174

dleehr · 2018-08-06T17:23:06Z

app/components/delivery-breadcrumbs.js

  }),
  isResendSelected: Ember.computed('selectedRouteName', function () {
    return this.get('selectedRouteName') === 'deliveries.show.resend';
-  })
+  }),
+  isNewDeliverySelected: Ember.computed('selectedRouteName', function () {


You can use Ember.computed.equal here

dleehr · 2018-08-06T17:24:20Z

app/components/delivery-breadcrumbs.js

@@ -6,9 +6,13 @@ export default Ember.Component.extend({
  transfer: null, /* specific transfer to be passed to child breadcrumbs */
  selectedRouteName: null, /* route we should select ('deliveries.index', 'deliveries.show', etc) */
  isDeliverySelected: Ember.computed('selectedRouteName', function () {


I don't know that a regex is easier to read than indexOf == 0, but FYI there's Ember.computed.match

Instead of filtering by deliverable filters by is_deleted to work around ember DS caching issues.

Adds some code to show errors.

This test only fails on travis.

johnbradley · 2018-08-10T13:02:56Z

I simplified this PR by removing the optional step of picking share users. I will create an issue to implement this feature in a future PR.

dleehr

Nice improvements. Some suggestions for Ember practices that I've found to make things easier and didn't catch on the first review.

dleehr · 2018-08-10T17:17:08Z

app/controllers/deliveries/new/select-project.js

@@ -24,7 +26,7 @@ export default Ember.Controller.extend({
    }
  }),
  currentUserCanDeliver: Ember.computed('currentUserProjectAuthRole', function () {


Can't this be Ember.computed.equal()?

dleehr · 2018-08-10T17:22:48Z

app/templates/components/duke-ds-project-list.hbs

@@ -6,6 +6,7 @@
  showGlobalFilter=false
  showPageSize=false
  pageSize=12
+  filteringIgnoreCase=true


Wish I had known about this before!

dleehr · 2018-08-10T17:45:54Z

app/adapters/duke-ds-project-permission.js

+import ApplicationAdapter from './application';
+
+export default ApplicationAdapter.extend({
+  urlForQuery(query) {


I think I follow this, but I wanted to clarify.

In the discussion of Duke-GCB/D4S2#170, I argued that it didn't make sense for the API to have a required query parameter to fetch project permissions. I recognize that, without a proper hasMany relationship to get the related records, Ember Data offers store.query() to do fetch a filtered list for a resource, and this code overrides the default query implementation to work with the project detail route in Duke-GCB/D4S2#170. I think it's probably worth a comment about why this is here.

I'll follow up with an issue, but I wanted to have a discussion on whether or not it makes sense to just make ajax calls for the project permissions instead of shoehorning them into ember data models.

My understanding is the hasMany relationship requires a top level endpoint to get the related keys (ie. /duke-ds-project-permission/<id>). I'm all for just making an ajax call, but not sure how easy it would be to re-use the emberjs auth.

Yeah I think we're on the same page - I was mostly trying to think out loud as to why this is done this way. Through the course of that I think I came to the conclusion that this doesn't fit 100% in a vanilla Ember Data store, but there are ways to make an authenticated AJAX call here and customize how the response is handled. We do it in bespin-ui.

I added a comment to the adapter.

dleehr · 2018-08-10T17:46:09Z

app/controllers/deliveries/new/select-project.js

+    }
+    return this.get('store').query('duke-ds-project-permission', {
+      'project': projectId,
+      'user': currentUserId,


The user parameter is not used, and we don't need quotes around the key names.

The user parameter is added to the end of the url and filters the permissions to just those for the current user. Otherwise the firstObject might be another user's permissions.

I see, so my understanding of the urlForQuery method wasn’t complete. This query parameter still gets passed through

Yep. That's why I had to delete query.project; in urlForQuery or it ended up being used twice.

dleehr · 2018-08-10T18:04:19Z

app/controllers/deliveries/new/select-project.js

+  project: null,
+  application: Ember.inject.controller(),
+  currentDukeDsUser: Ember.computed.alias('application.currentDukeDsUser'),
+  currentUserProjectPermissions: Ember.computed('project', 'currentDukeDsUser.id', function () {


I see several instances where computed properties are returning promises (e.g. the result of a store.query()), which can make you want to tear your hair out. I know it's given me a lot of grief during development and testing, and I'm guessing it's why these functions have so many early bail outs.

I'd encourage you to have a look at that article to see if any of those patterns apply, and I'll mention that I've had good results with setting the property after the promise resolves:

For controllers, you can add an observer on init (or some other property). This is how the application controller sets the current user:

datadelivery-ui/app/controllers/application.js

Lines 7 to 16 in 86edec2

authenticatedDidChange: Ember.on('init', Ember.observer('session.isAuthenticated', function () {

if (this.get('session.isAuthenticated')) {

this.get('dukeDsUser').currentDukeDsUser().then(currentDukeDsUser => {

this.set('currentDukeDsUser', currentDukeDsUser);

});

} else {

this.set('currentDukeDsUser', null);

}

})

)

For components, kick off the promises in didInsertElement(), since they don't have the same object lifecycle as other objects (via https://emberigniter.com/render-promise-before-it-resolves/):

https://github.com/Duke-GCB/bespin-ui/blob/9d25758fb6d82c7e46f309bee248877b239685c8/app/components/questionnaire/dds-project-field.js#L12-L21

That guide says that there is a danger with this approach (#1 in the guide):

waiting for promises and this.setting – on a potentially destroyed component

I'm going to use the #3 Special promise-backed objects option.

dleehr · 2018-08-10T18:10:39Z

app/models/duke-ds-project-permission.js

+export default DS.Model.extend({
+  project: DS.belongsTo('DukeDsProject'),
+  user: DS.belongsTo('DukeDsUser'),
+  auth_role: DS.attr('string')


Should be camelCase

dleehr · 2018-08-10T18:19:28Z

app/controllers/deliveries/new/select-recipient.js

+  currentDukeDsUser: Ember.computed.alias('application.currentDukeDsUser'),
+  validUsersList: Ember.computed('model.[]', function () {
+    // remove users with invalid fullName values
+    return this.get('model')


I see users in my listing with empty email addresses too. Won't this be problematic too?

I added code to filter out users without emails.

dleehr · 2018-08-10T18:27:23Z

app/controllers/deliveries/new/select-recipient.js

+  actions: {
+    toUserSelectionChanged(actionData) {
+      var selectedItem = null;
+      if (actionData.selectedItems) {


One benefit of Ember is that arrays (and other enumerables have additional, observable properties like firstObject and lastObject that don't raise errors for empty arrays.

So, rather than having to check the length of the array first and then index into it, you could reduce the entire body to

this.set('toUser', selectedItems.get('firstObject'));

dleehr · 2018-08-10T18:28:39Z

app/controllers/deliveries/new/select-project.js

+    });
+  }),
+  currentUserProjectAuthRole: Ember.computed('currentUserProjectPermissions.[]', function () {
+    const authRoles = this.get('currentUserProjectPermissions').mapBy('auth_role');


Can also use .get('firstObject') here

dleehr · 2018-08-10T18:28:55Z

app/controllers/deliveries/new/select-project.js

+  }),
+  actions: {
+    projectSelectionChanged(actionData) {
+      var selectedItem = null;


See other comments about firstObject

johnbradley added 10 commits July 13, 2018 16:50

Merge branch 'master' of github.com:Duke-GCB/datadelivery-ui into 6-n…

d52c6c5

…ew-deliveries

checkbox for selecting and other cleanup

708831c

Adds checkboxes for selecting projects and users. Filters users to exclude current user.

tests for new components

695fa85

also fixes failing new tests for controllers

remove unused model

466b672

remove deliveries controller

cb44c00

application now contains logic for currentDukeDsUser so this is no longer needed.

test delivery state field defaults to NEW

44781cd

queries project list with isDeliverable

50ca344

adds tests for new routes

tests for select-project and select-recipient controllers

d5f092f

Add back in RSVP.all for saveAndSend

dbbe482

Th computed field shareUsers returns a javascript array of promises so we can't just call `then` on the returned array.

johnbradley mentioned this pull request Jul 30, 2018

Add is_deliverable field to projects endpoint Duke-GCB/D4S2#169

Closed

johnbradley added 2 commits July 30, 2018 09:35

duke-ds-project isDeliverable -> is_deliverable

d543d4e

Switch to match existing pattern.

Add user instructions to the delivery new subroutes

6d77719

johnbradley requested a review from dleehr July 30, 2018 15:16

dleehr reviewed Aug 6, 2018

View reviewed changes

johnbradley added 13 commits August 8, 2018 16:03

check perms after user picks a project for delivery

15da3a8

Instead of filtering by deliverable filters by is_deleted to work around ember DS caching issues.

force reload of recipient list

2871f01

simplify breadcrumb computed properties

068ddab

remove share user selection from select-recipient

3bf70ad

Adds some code to show errors.

fix tests for removed shareUsers

2b9d464

add duke-ds-project model requirement for test

fecb96f

tweak messaging and layout of screens

377c191

add other model for test

193f960

This test only fails on travis.

filter out bad users and sort by fullName

e6ee8e2

test sorting by user Name

b493477

test changes for filtering out 'null' duke-ds-users

ecb4cc2

enable case-insensitive projects and users

4b821bb

remove unused queryParam

ff92e30

johnbradley added 4 commits August 9, 2018 16:15

test for select-project showMissingPrivilegesError

780c7b9

simplify select-project and add more tests

91c45b5

add newlines to end of two files

2ddd7d6

revert un-necessary pageSize change

1168694

reset errors when existing enter-user-message

ff42d3a

johnbradley requested a review from dleehr August 10, 2018 13:29

dleehr reviewed Aug 10, 2018

View reviewed changes

johnbradley added 4 commits August 10, 2018 15:35

minor improvements

cba5f81

authRole camel case changes

189d961

filter out users with empty emails

6f229f9

simplify controllers with ember shortcuts

2b4f7af

dleehr mentioned this pull request Aug 13, 2018

Clean up implementation of fetching project permissions #22

Closed

johnbradley added 3 commits August 13, 2018 10:13

add comment for duke-ds-project-permissio adapter

310d18d

wrap returned promises in DS.Promise* objects

34b2205

Update duke-ds-project-permission.js

cb9141d

johnbradley requested a review from dleehr August 13, 2018 17:09

dleehr approved these changes Aug 14, 2018

View reviewed changes

johnbradley merged commit 430d3fa into master Aug 14, 2018

johnbradley deleted the 6-new-deliveries branch August 14, 2018 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to create deliveries #21

Allow users to create deliveries #21

johnbradley commented Jul 26, 2018 •

edited

Loading

dleehr left a comment

dleehr Aug 6, 2018

johnbradley Aug 8, 2018

dleehr Aug 6, 2018

johnbradley Aug 8, 2018 •

edited

Loading

dleehr Aug 6, 2018

dleehr Aug 6, 2018

johnbradley commented Aug 10, 2018

dleehr left a comment

dleehr Aug 10, 2018

dleehr Aug 10, 2018

dleehr Aug 10, 2018

johnbradley Aug 13, 2018

dleehr Aug 13, 2018

dleehr Aug 13, 2018

johnbradley Aug 13, 2018

dleehr Aug 10, 2018

johnbradley Aug 10, 2018 •

edited

Loading

dleehr Aug 10, 2018

johnbradley Aug 10, 2018 •

edited

Loading

dleehr Aug 10, 2018

johnbradley Aug 13, 2018

johnbradley Aug 13, 2018

dleehr Aug 10, 2018

dleehr Aug 10, 2018

dleehr Aug 10, 2018

johnbradley Aug 10, 2018

dleehr Aug 10, 2018

dleehr Aug 10, 2018

dleehr Aug 10, 2018

	authenticatedDidChange: Ember.on('init', Ember.observer('session.isAuthenticated', function () {
	if (this.get('session.isAuthenticated')) {
	this.get('dukeDsUser').currentDukeDsUser().then(currentDukeDsUser => {
	this.set('currentDukeDsUser', currentDukeDsUser);
	});
	} else {
	this.set('currentDukeDsUser', null);
	}
	})
	)

Allow users to create deliveries #21

Allow users to create deliveries #21

Conversation

johnbradley commented Jul 26, 2018 • edited Loading

dleehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbradley Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbradley commented Aug 10, 2018

dleehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbradley Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbradley Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbradley commented Jul 26, 2018 •

edited

Loading

johnbradley Aug 8, 2018 •

edited

Loading

johnbradley Aug 10, 2018 •

edited

Loading

johnbradley Aug 10, 2018 •

edited

Loading