Yes. The intent is to provide a resource through the API to deliver the data needed to produce reports/graphs in the client (browser). If in the future we need to protect the server from servicing too many calls, we could cache the data (either in an internal table or, more likely, just using nginx caching for the resource).
The main point is to inform the client/user when an error has occurred. If there are SQL failures, then “Unable to retrieve xyz from the database” or simply “An unexpected database error occurred. Please ask an administrator to check atlas logs at YYYY-DD-MM HH:MM:SS for more information” may suffice. What currently happens if you force one of those SQL calls to fail? I’m assuming that the http request will either hang & timeout, fail with some default express failure message, or – worst case – return as 200 OK without the expected result. I haven’t looked at them all, but wouldn’t be surprised if there are situations where subsequent code would be executed inappropriately because we don’t return after logging these errors and many are nested several layers in if/then/else conditions where it might not be able to see whether the console log line will be the last line executed in the method.
I updated the PR with samples of the being JSON returned, are they ok?
Aye! I was just a little confused about the "A useful message that would allow the client to understand what exactly failed and what they might do to fix it" bit. I thought we were supposed to check for all possible input errors the user migth’ve made (if statement bad-place). I also updated ATLAS-202. I made a primitive commit for the former ticket but it’d be easier to make a PR and avoid conflicts once ATLAS-202 gets merged as all the console.logs change to logger.somethings.
I got a mail recently about GSoC, mentioning that I may need to eventually submit a project page explaining my work. What do you think I should submit then, this talk page, the project page(I’m assuming its this one) or perhaps my blog?
We will want the API to supply data for a few different reports in the future, so don’t use /api/report as a resource itself. Rather, use /api/report/modules as the resource for getting this “modules” report data.
In the JSON, don’t use module names as keys. Instead of entries like:
We want these to be an array of modules, so the first & last characters would be [ and ].
FYI – You should only be including the “active” modules in these counts.
I wouldn’t use a path like GET /api/report/Atlas Module. First off, it’s not a valid path. You’d have to use something like GET /api/report/Atlas+Module or GET /api/report/Atlas%20Module to make it valid. Also, it’s better to use IDs instead of names for identifying resources. And, as I mentioned earlier, I’d like the resources under /api/report/* to represent different reports. We’ll start with modules, but we could have other reports like “activity” to report on Atlas marker activity (though we have a nifty RSS feed for that), “usage” to provide some data on Atlas website usage, etc. (I’m just making these up as examples… for now, all we need is a “modules” report). If we want to support filtering to a specific module, we could either use GET /api/report/module/:module_id (i.e., GET /api/report/module/reporting) or GET /api/report/module?id=:module_id. I think the first one GET /api/report/module/:module_id is more RESTful, especially since the “module” resource would return an array of results and adding the module ID to the path would filter to that one result. But filtering to a specific module isn’t really needed, since it will actually be easier on the server to just provide the full set of data all at once rather than handling individual requests for each module.
I would suggest using the short link to your wiki project page – i.e., https://wiki.openmrs.org/x/I4DYCg, since that page should be updated and contain links to your blog and this discussion.
By the way, I tried deploying some or your latest changes to staging and ran into a couple problems. First, the addition of a google analytics tracking id and our change to the API path requiring an update to the health check in docker-compose means we need to redeploy docker-compose for atlas. I reached out to @cintiadr on our telegram infra channel to ask how to do this properly. Also, atlas on staging is now crashing whenever you try to log in. It appears to be reporting a certificate error as it tries to authenticate via ldap:
Error: certificate has expired
at TLSSocket.<anonymous> (_tls_wrap.js:1116:38)
at emitNone (events.js:106:13)
at TLSSocket.emit (events.js:208:7)
at TLSSocket._finishInit (_tls_wrap.js:643:8)
at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:473:38)
I don’t know if this is a bug we’ve introduced or if there’s something misconfigured with ldap staging. https://ldap-stg.openmrs.org/ gives a nginx gateway error, but it may always have done that (since we are using ldaps & not https) and, despite that error, the TLS certificate for ldap-stg doesn’t appear to have expired.
It’s nearly 2am here and I’m losing consciousness involuntarily, so will look to you and/or @cintia to lend a hand or I’ll try to get this sorted tomorrow or over the weekend.
Update: I did a little more digging, was able to recreate the ldap authentication failure locally, found this tip to add userClient.on('error', function(err) { ... }); to capture the specific ldap error, which looks like this:
Which, unfortunately, doesn’t immediately point me to a solution. It looks as if some new defect in atlas is keeping it from finding the ldap server. Hopefully, @heliostrike, you can recreate the error yourself and maybe figure out what we did to introduce this problem.
Update 2 (now I really need some sleep): it looks like my local issues were related to a change to networks@cintiadr made to the local docker-compose for atlas. When I removed her network changes, my local version of atlas worked again. I’m not sure (but suspicious) if those changes might also be preventing atlas-stg from seeing ldap.
I forgot that json header could return arrays, which is why changed the API that way (._. ; ). I blame the night for that! I updated ATLAS-206 accordingly and updated the sample responses.
My expertise on troubleshooting server errors doesn’t extend much beyond google searches, but I’ll take a look around.
Updates:
Googling didn’t give me any pointers I could use. Deploying a previous commit to staging is taking an unusual amount of time. Usually it takes about 30s but now its been running for 10 min with the following log repeating.
09-Aug-2019 10:07:07 Verifying if docker containers 134cfa1b757929688c113119ee9fe11ee62312a0dc817068cbf90127d7c52174
09-Aug-2019 10:07:07 cc643a1cbab3cf207dd747515632f60c73179d52fb0a08581ee27559f14d6c6a
09-Aug-2019 10:07:07 6a03dc245df60a7b1a4424bc741a4d51d4744fc682ca0d0c33befb9842d75e22 are healthy
09-Aug-2019 10:07:07 Status found: "healthy" "unhealthy"
Finally, the deployment failed.
@cintiadr suggested to destroy and start ldap-stg, as its certificate expired.
The search box doesn’t render properly for me on Chrome on Mac or using Safari on my iPhone:
While the “only show markers that match” approach is a natural approach to searching, it has several drawbacks (e.g., what to do for markers that are hidden due to fading, markers can be off the screen depending on zoom level, etc.). Also, the search box + search button UX can be confusing (e.g., I can be searching with an empty text box or not searching when text is in the box). Instead, I was imagining the search button would bring up a modal text box. Typing in the search box would show a list of real time matches in a list below and selecting any item from the list would take you to its marker.
I’d favor using the / key to jump to search, since it’s a more common convention. Alternatively, you could hook the search key (Ctrl/Cmd+F) like Discourse does for long topics.
Great start! I’m hoping we can evolve it toward something like this:
I updated the search feature to follow the drop-down fashion, but have yet to convert it into a modal.
Is Ctrl alright? Looks like mozilla is catching all ‘/’ keypresses to open up its own search box, but Ctrl works fine in mozilla and chrome.
That’s an amazing mockup! Now that I know what exactly we’re looking for, it shouldn’t be too hard to whip up a similar UI. Do I update the current PR, or should we postpone it to a future PR?
I updated the search PR to use a modal and deployed it on staging. I’m not very good at deciding whether something looks alright aesthetically, so I think I need some help with that. Ctrl also opens up the search bar.
Thanks. This is looking better. I’d suggest making the search box 80-90% of the width of the modal window and show appropriate marker icons for each site.
Don’t forget user-select: none and cursor: pointer for choice CSS
When search box gains focus, do a select() to select all text to existing text will be replaced by next text by default (re-opening search box and finding your last search is fine, but invoking search and starting to type should not add text to the end of the previous search by default).
If the user selects an invisible site (i.e., fully faded), you need to turn off fading (automatically toggle off fading altogether, instead of making a marker visible that shouldn’t be visible when fading is enabled).
The search modal should be dismissed when you select from the list.
I’m not a fan of using the Ctrl key for search. It’s non-standard and interferes with other normal behaviors (e.g., switching the browser to full screen using Ctrl+Cmd+F on a Mac invokes a search). Can we instead capture Ctrl+F (Cmd+F on Mac)? That’s the standard search key and there’s little utility for the native search.
Either is fine w/ me. You can just update the existing PR. Have you seen plot.ly?
BTW… I merged your module PRs, which caused conflicts in the other PRs. Sorry 'bout that.
What do you use to create such amazing mockups?!?!?!?!
I updated the PR with the necessary changed, but I didn’t entirely understand this point.
I updated the PR with select(), but didn’t quite get the text after that.
I feel a bit stupid for not doing this earlier, but turns out event.preventDefault() fixes the issue we had with ‘/’ in mozilla. So… is ‘/’ ok? :'p
Tomorrow’s Sunday, so I’ll make sure to update it by the end of the day (assuming I won’t hit too many road blocks). Also, we would want buttons for the RSS feed and the data page on the top-right corner of atlas page, wouldn’t we?
Merge conflicts give me something to do, so that’s nice (if only resolving them would give github contributions! I need more green!). Anyway, fixed them.
I updated the data page with UI resembling your mockup. What about the page do you think could be improved? The search markers feature is also deployed on staging.
It looks great, @heliostrike! The labels appear a little wonky to me (cropped, running right up against the graph with no padding, and/or not centered on the corresponding line of the y axis).
I was just saying that, without selecting text on focus, if you searched more than once, the cursor was sitting at the end of your prior search text. So, searching for “foo” and then “bar” would actually search for “foobar” if you didn’t manually remove the “foo” before typing your second search term. In any case, you fixed it.
Yes! Works great. Did you think to add a tooltip on the search button? e.g.: Press “/” to search.
I’d put a small RSS logo in the bottom right corner. We’re getting too many control buttons. Let’s put a link to the data page into the help text like this:
“This map is updated with information from OpenMRS users around the world. Summary statistics are available here . To add or edit information, you’ll …”
BTW, I added a few more tickets in JIRA for issues I’ve found and left a comment on the remaining open PR. Once we get that PR merged, I think it’ll be your turn to promote to production! We might get in a couple of these little things in as well if you get to them, but I wouldn’t hold up promoting to production for any of them.
Strong work, @heliostrike! I deployed build #100 to staging. So far, it’s working great for me. If you agree things are working, then please promote to production. I think it’s a perfect last week activity for you to be promoting your great work to production.
I knew I was in over my head.
