I use docker compose to run the stack with these containers:
db runs the database
api uses node to serve the API
web uses nginx to serve the web app
admin uses nginx to serve the admin web app
An nginx instance could be set up to serve the web app at root and the admin app at /admin… or the web container can be configured to proxy the admin app at /admin.
I’d like admins to be able to edit any marker.
@heliostrike, could you prioritize this? That is, if the user is an admin (member of atlas-admin group in ldap), they should be able to edit/delete any marker.
Expected behavior:
Log in as test and create a marker; test can edit the marker
Log in as testadmin and you should be able to edit test’s marker
Add a marker as testadmin and you should be able to edit all markers (made by testadmin or test)
Log bakc in as test and only test’s marker(s) are editable (markers made by testadmin can be viewed by aren’t editable by test)
This was the original behavior, so hopefully the client is already ready to show markers as editable for admins and may just need to be tweaked to use the user’s admin property. Assuming you can get this working and we can validate the behavior in staging, then we could promote to production (i.e., upgrade production atlas to 3.x!).
Looking a little closer at markerSites.js, it looks like there’s a bit more work needed:
Changes (e.g., update and delete) should validate the user’s access prior to applying changes to the database. For example, instead of adding the current user as creator criteria when issuing a DELETE in the database, we should verify the user is an admin or is an owner of the marker before we’re issuing a DELETE command in SQL (i.e., don’t just depend on SQL to do the right thing). In other words, first verify that the user should be able to change the marker (returning a 401 Unauthorized if not) and only if that check passes, proceed with issuing an INSERT, UPDATE, or DELETE command to the database (including criteria in SQL as an extra layer of safety is fine… but shouldn’t be the primary way we enforce access).
I’m not sure I agree with filtering markers when the user is authenticated. Users should be able to see the Atlas (with all sites) even when authenticated.
It looks like we’re performing SELECT * FROM atlas and returns all data in JSON in several places; however, if someone managing a maker has opted out of sharing counts (i.e., show_counts is false), then counts (patients, encounters, and observations) should not be shared with people who do not have edit access for the marker.
markerSites.js should probably be renamed to markers.js
We’re filtering out the marker IDs, not the markers. All markers will be visible to everyone, even if they’re not signed in. We agreed on not returning the marker IDs of markers that do not belong to the user a while ago, as marker IDs are necessary to update/delete markers. I think this bit can be removed after adding checks to check the user’s authenticity.
Is this ok?
// If user if not authenticated
// or username in session is not the same as what's in the request
// or user is not admin
// return 401
if(!req.session.user || (req.session.user.uid != req.body.uid && !req.session.user.admin)) res.send(401);
@burke@harsha89@cintiadr I’ll make the requested changes but I think that some of the changes may conflict with some of the existing PRs(#55, #50).
I think that using nginx and creating more containers would just complicate the codebase. A single page application would be nice, but I think creating /admin and extending it forward using Node routes would maintain the simplicity of the codebase (and would be much easier to implement).
If we’d like a single page application, I suppose it can be done without using another container, by importing React and JSX source files into a html page, and treating it like a normal html page. But unless there’s a good reason to prefer a single page application, I feel like we’ll just be complicating things by moving ahead with one.
It’s a fair point. I guess, being a fan of dotadiw, I tend to favor separation of concerns – e.g., “make an API then use the API” rather than make an application that is an amalgam of API, web app, and admin app. Given the size and near-term requirements for the Atlas, it probably doesn’t matter. I would still suggest we approach the web application components as client-side apps – i.e., avoid server-based web pages that bypass use of the API – both to avoid spending server cycles building web pages and to support other uses of the API by ensuring everything Atlas does is reflected in its API.
I agree. Is it ok if I create a normal webpage (rendered using Node) and import React into it, to create the single page application, rather than use nginx?
Certainly! The primary thing I’d like to avoid is node server pages (i.e., writing node scripts that respond to web page requests and query the database directly while generating the response). Whether a web page or CSS files are served up by the node app or nginx doesn’t really matter based on our requirements/needs for Atlas.
@burke@harsha89@cintiadr I’ve spent quite some time trying to setup react for the admin page, but it turned out to be more troublesome than I expected.
Is it ok if I could build the admin functionality using node routes for now so that we could start working with the types, versions, and distributions API? I will try to get react working on the side, so that we could deploy it on a later date. Building the admin side using node shouldn’t take more than 2 days, so not a lot of time will be lost.
Is it possible to change the default “fade” setting so that the map does not fade out sites? It is a little disheartening to see so few sites actually mapped due to lack of updating…
The solution to fading is the purpose of this GSoC project – i.e., fix Atlas so sites can be updated & added and we can show current data.
Currently, sites fade 25% for every 6 months of not being updated (disappearing after 2 years). I think it would be reasonable to change this to start fading after a year (instead of 6 months) and then fade 25% per 6 months (disappearing after 2.5 years).
As @heliostrike said, you can always disable fading in the menu to see older sites.
ATLAS-151 - Create admin routes and interface to manage types
ATLAS-152 - Create admin routes and interface to manage versions
ATLAS-153 - Create admin routes and interface to manage distributions
I linked images of the features in each ticket. I didn’t create PRs for ATLAS-151, ATLAS-152, and ATLAS-153, as the API calls haven’t been tested, and there’s a tiny amount of work left on ATLAS-153 (turning ‘Is Standard?’ into a radio/dropdown input, instead of a text).
I’ll make PRs for them after ATLAS-145 (after testing that the API calls are working), and ATLAS-148 get merged, and after adding a little styling.
What do you think of the this, and is there anything you’d like to see changed?
Currently there’s to be no ‘show counts’ checkbox, when a marker.is being edited. On inspecting the code, it looks like the checkbox only appears on a site if site.module === 1. I’m not sure what is means, but I assume it means that the site was created by a module. I think we should allow owners and admins to decide whether to show counts. What do you think?
Also, how do you think that hiding of counts should be implemented?
@burke@harsha89@cintiadr I created a PR to get show_counts working. If show_counts is on, anyone see the marker’s patient/encounters/obs counts, else, only the creator and admins can see it.
Currently, the code hides counts on the front-end. How do I hide them, coming from the backend?
When someone runs the Atlas module, it helps them construct their marker and – in the process – gives them the option to opt-out of sending stats to OpenMRS Atlas. show_counts defaults to 1; if the user opts out of sending stats, the module sets show_counts to 0.
When someone comes to the OpenMRS Atlas website to create a marker, they are entering data manually and it’s presumed they are only going to share data that they want to share – i.e., a site will either be fine with sharing stats and will enter them in the form while creating their marker OR they will not care to share their stats, so won’t enter those fields. Having a “Do you want to show counts?” option while manually creating a marker would only apply if a user was willing to share stats privately (i.e., willing to tell us, but doesn’t want them displayed publicly). That latter scenario is an unlikely edge case and complicates the UX, so we just take the approach of “if you are putting something into the Atlas manually, people will be able to see it.”
In other words, the module automatically sends stats (e.g., weekly) and the user can opt-out of having those displayed on the Atlas website. When creating a marker manually through the Atlas website, users are entering data manually, so they demonstrate whether or not they want them shown by either entering them or skipping them.
How about something like:
SELECT
id, -- assuming we implement update tokens
latitude,
longitude,
name,
url,
type,
image,
show_counts,
IF(show_counts, patients, null) AS patients,
IF(show_counts, encounters, null) AS encounters,
IF(show_counts, observations, null) as observations,
contact,
email,
notes,
data,
atlas_version,
openmrs_version,
distribution,
date_created,
date_changed,
created_by
FROM atlas;
