mrc-357 add autocomplete and validate selected users #106

hillalex · 2019-07-10T10:38:12Z

~~Contains commits from #105 so merge that one first~~

Found a nifty bootstrap typeahead vue component for this.

codecov · 2019-07-10T11:00:34Z

Codecov Report

Merging #106 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #106      +/-   ##
============================================
+ Coverage     95.03%   95.07%   +0.04%     
- Complexity      518      519       +1     
============================================
  Files           141      141              
  Lines          2455     2479      +24     
  Branches        184      185       +1     
============================================
+ Hits           2333     2357      +24     
  Misses           77       77              
  Partials         45       45

Impacted Files	Coverage Δ	Complexity Δ
...rlyweb/app_start/routing/web/WebUserRouteConfig.kt	`100% <100%> (ø)`	`2 <0> (ø)`	⬇️
.../org/vaccineimpact/orderlyweb/db/UserRepository.kt	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...ic/src/js/components/reports/reportReadersList.vue	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...mpact/orderlyweb/controllers/web/UserController.kt	`100% <100%> (ø)`	`5 <1> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41356ff...9f01535. Read the comment docs.

hillalex · 2019-07-10T15:42:43Z

...app/src/main/kotlin/org/vaccineimpact/orderlyweb/app_start/routing/web/WebUserRouteConfig.kt

                .transform()
-                .secure(usersManage)
+                .secure(usersManage),
+            WebEndpoint("/users/",


Should this be /users/ or something more specific, like /emails/? It's just for returning valid user emails for the autocomplete...

If it's just returning emails I'd probably expect a more specific route, since we already have endpoint which are returning user models, which in a RESTful way I'd expect to be the resource I'd find at /users/. But since this isn't a part of the REST API and is just for the web app's internal use I guess it isn't so important!

Actually, having said that, maybe we don't have anything returning the User model just now, as there's now a ReportReaderViewModel instead. Although maybe that will change once we've done the Admin screens.
I still think /users/emails might be a bit clearer!

EmmaLRussell

Yeah, that bootstrap-typeahead is nice, makes life a lot easier!

It might be nice if, after you've added a new user, it cleared the email control, so you could just click back in there and start typing to get more auto complete results, rather than having to clear the box first.
Ooh actually, having said that, I've just noticed that you do get a little blue cross (that's come from bootstrap-typeahead I guess) which makes that easy to do anyway.

EmmaLRussell · 2019-07-11T15:24:49Z

...app/src/main/kotlin/org/vaccineimpact/orderlyweb/app_start/routing/web/WebUserRouteConfig.kt

                .transform()
-                .secure(usersManage)
+                .secure(usersManage),
+            WebEndpoint("/users/",


If it's just returning emails I'd probably expect a more specific route, since we already have endpoint which are returning user models, which in a RESTful way I'd expect to be the resource I'd find at /users/. But since this isn't a part of the REST API and is just for the web app's internal use I guess it isn't so important!

EmmaLRussell · 2019-07-11T15:25:47Z

...app/src/main/kotlin/org/vaccineimpact/orderlyweb/app_start/routing/web/WebUserRouteConfig.kt

                .transform()
-                .secure(usersManage)
+                .secure(usersManage),
+            WebEndpoint("/users/",


Actually, having said that, maybe we don't have anything returning the User model just now, as there's now a ReportReaderViewModel instead. Although maybe that will change once we've done the Admin screens.
I still think /users/emails might be a bit clearer!

EmmaLRussell · 2019-07-11T15:31:05Z

src/app/src/main/kotlin/org/vaccineimpact/orderlyweb/controllers/web/UserController.kt


+    fun getUserEmails(): List<String>
+    {
+        return userRepo.getUserEmails()


So we're assuming that we always have a manageable number of users to be able to return the full list on each call? No value in doing an initial filter with first few letters here? Probably not for the number we have in Montagu, would just make the UI less responsive.

I think so, yeah. The number of users would have to be pretty high for this to be a problem.

EmmaLRussell · 2019-07-11T15:33:54Z

...app/src/test/kotlin/org/vaccineimpact/orderlyweb/tests/database_tests/UserRepositoryTests.kt

    }

+    @Test
+    fun `gets user emails in alphabetical order`()


Is this test name misleading? I can't see ordering code, and I didn't think containsExactlyElementsOf checked order.

containsExactlyElementsOf is order sensitive. Seems sqlite returns the records in alphabetical order, hence no extra ordering logic.

hillalex · 2019-07-11T16:06:50Z

Ok, /user/emails/ would end up giving us ambiguity problems if we wanted to implement /user/:user/ so I've gone with the super specific /typeahead/emails/. That way we'll always be clear about why we have this route! To be shortly followed by /typeahead/roles/ (and I guess at some point /typeahead/permissions.. reports... versions etc)

EmmaLRussell

The first commit commit implies that the intention was to clear the email field after successful add, but it doesn't seem to. Not sure if I'm missing something!

hillalex · 2019-07-12T11:11:21Z

Huh, ok, what I thought was the fix is this typo fixed here:
9b8a79e#diff-9b4909ce9cecd056a2e47e2dac8a916aR105 and tested here: 9b8a79e#diff-4ff6d71081ba63d542dd80f1a3ec4850R174

(slightly confusingly I also fixed the naming convention from camel case to snake case at the same time)

But it seems like resetting the v-model of the typeahead doesn't actually trigger a change in that component; in fact looks like they have an open PR that fixes this, so I''ll keep an eye on that and update once fixed: alexurquhart/vue-bootstrap-typeahead#21

hillalex · 2019-07-12T11:30:28Z

Oh man, actually looks like that repo is dead. Last commit in October and lots of outstanding PRs. Maybe we should fork it and fix it.

hillalex added 3 commits July 9, 2019 18:38

add typeahead and validation

42fe81d

update js tests

e924a22

Merge branch 'mrc-356' into mrc-357

9d554a3

hillalex marked this pull request as ready for review July 10, 2019 11:09

hillalex requested a review from EmmaLRussell July 10, 2019 11:18

Merge branch 'master' into mrc-357

3ab153d

hillalex removed the request for review from EmmaLRussell July 10, 2019 15:33

tests for new /users/ endpoint

3937529

hillalex commented Jul 10, 2019

View reviewed changes

merged master

76aecd1

hillalex requested a review from EmmaLRussell July 10, 2019 16:21

hillalex mentioned this pull request Jul 11, 2019

mrc-353 frontend #108

Closed

EmmaLRussell requested changes Jul 11, 2019

View reviewed changes

change route and clear email after successful add

9b8a79e

change route to be even more specific

1a81cc2

hillalex requested a review from EmmaLRussell July 11, 2019 17:03

EmmaLRussell reviewed Jul 12, 2019

View reviewed changes

merged master

9f01535

hillalex requested a review from EmmaLRussell July 12, 2019 11:32

hillalex mentioned this pull request Jul 12, 2019

mrc-360 - preliminary refactor #109

Merged

EmmaLRussell approved these changes Jul 12, 2019

View reviewed changes

EmmaLRussell merged commit f0c58b7 into master Jul 12, 2019

mrc-357 add autocomplete and validate selected users #106

mrc-357 add autocomplete and validate selected users #106

Uh oh!

Conversation

hillalex commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EmmaLRussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hillalex commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EmmaLRussell left a comment

Choose a reason for hiding this comment

Uh oh!

hillalex commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hillalex commented Jul 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hillalex commented Jul 10, 2019 •

edited

Loading

codecov bot commented Jul 10, 2019 •

edited

Loading

hillalex commented Jul 11, 2019 •

edited

Loading

hillalex commented Jul 12, 2019 •

edited

Loading