DASSL / Gradebook

Open-source product to provide a practical means for instructors to record student attendance and assessment
Other
8 stars 4 forks source link

Add Gradebook web server #30

Closed wildtayne closed 7 years ago

wildtayne commented 7 years ago

This adds an initial version of the Gradebook web server. The following URL handlers are implemented:

smurthys commented 7 years ago
afig commented 7 years ago

Edit:

Important typos

Line 148 (should be "seasonname" rather than "sesonname"):

"sesonname": result.rows[row].seasonname

Line 184 (should be "courses" rather than "Courses"):

 "Courses": courses

Line 222 (similar issue):

"Sections": sections

Note: Although it would normally be acceptable for "Courses" and "Sections" to have a capital letter, it is not consistent with the other REST calls


Apart from the changes that @smurthys suggested, we also probably want to add URL handlers for the css and js dependencies of the main page (index.html):

The above paths follow the structure that we currently have set up on development instances:

  webapp/
  |--css/
  |  |--materialize.min.css
  |
  |--js/
  |  |--materialize.min.js
  |  |--index.js
  |  |--gradebookServer.js    (file added w/ this PR)
  |
  |--index.html

Currently, gradebookServer.js searches for index.html in the same directory it is being run from. This does not currently work with the development environment. However, we may choose to move gradebookServer.js to its parent directory in which case the development directory structure will be:

  webapp/
  |--css/
  |  |--materialize.min.css
  |
  |--js/
  |  |--materialize.min.js
  |  |--index.js
  |
  |--gradebookServer.js    (file added w/ this PR)
  |--index.html

The choice we make will affect the code that will be added to serve the dependencies outlined above. Here is some sample code that could be added to gradebookServer.js, provided we go with the second choice of directory structure.

//Serve css and js dependencies
app.get('/css/materialize.min.css', function(request, response) {
    response.sendFile('css/materialize.min.css', {root: __dirname});
});

app.get('/js/materialize.min.js', function(request, response) {
    response.sendFile('js/materialize.min.js', {root: __dirname});
});

app.get('/js/attendance.js', function(request, response) {
    response.sendFile('js/attendance.js', {root: __dirname});
});
smurthys commented 7 years ago

Thanks @afig especially for the notes on directory structure.

I confess I have yet to fully understood the deployment models and constraints of a node server, but it will be important to maintain the server and the client in separate folders: ideally sibling folders, but one as a sub-folder as the other will also work. In any case, we should strive to make the repo org the same as the deployment org. I have a feeling the latter org is practical.

If the client and server are stored in sibling folders, I propose directories webClient and webServer as is the current thought for the repo directories. In this case, the server needs access to its parent directory. I am not sure if that is possible. In fact, many retail web hosting services don't allow such access. Even if this is possible in a "fully owned" OS instance as is the case during development, we should not assume the same for deployment for money reasons.

 webClient/
 |--css/
 |  |--materialize.min.css
 |
 |--js/
 |  |--materialize.min.js
 |  |--index.js
 |
 |--index.html
 |--attendance.html        (example of another html file the client uses)
 |
 webServer/
 |--sub-dir/               (example of a sub-directory the server uses)
 |  |--file-in-sub-dir
 |
 |--gradebookServer.js     (file added w/ this PR)

An alternative organization (and one I'd probably recommend as of now) is to store the server in a webapp directory and the client files in a client sub-directory. In this case, the server needs to prefix each directory in the url requested with client/

 webapp/
 |
 |--client/
 |  |--css/
 |  |--materialize.min.css
 |
 |  |--js/
 |  |--materialize.min.js
 |  |--index.js
 |
 |  |--index.html
 |  |--attendance.html     (example of another html file the client uses)
 |
 |--server-sub-dir/        (example of a sub-directory the server uses)
 |  |--file-in-server-sub-dir
 |
 |--gradebookServer.js     (file added w/ this PR)
wildtayne commented 7 years ago

I am also not sure what limitations are placed on node.js PasS deployments, so I can't really speak to which limitations we need to consider. As far as the program itself is concerned, either suggested directory structure is OK, since we have to explicitly map each file on the server to a URL. For now, I agree the second structure proposed by Dr. Murthy is the most flexible.

One limitation imposed by node.js on specifying file paths is that relative path names can't be directly used. For example, the following call causes an error: response.sendFile('../index.html'); One way to work around this is to supply the root parameter, for example: response.sendFile('index.html', {root: __dirname + '../'});

I used this in the current version of gradebookServer, since the summer2017 repo stores client files in the parent directory of the server. If we do use the second proposed directory structure, I will remove the use of {root: __dirname}, as it will not be necessary.

wildtayne commented 7 years ago

After some further experimentation, I have found that setting the root parameter is still necessary with the proposed directory structure. Node.js seems to want the full path of the file be served, unless that file is in the same directory as the server's js file. So, for serving files in the client folder, we still need to specify {root: __dirname}

wildtayne commented 7 years ago

I agree this should be merged after #32 and #35 (and possibly #33), since the server depends on these.

I just pushed some commits to address issues from this thread:

smurthys commented 7 years ago

I am OK using the second directory org I proposed if @afig is also ok with it.

A few observations:

afig commented 7 years ago

I am okay with the second directory organization that @smurthys has proposed. The comments and revisions made so far look good.

One thing I could add is that the server is currently directed to listen on port 3000. We could change this to port 80 so as to not require a tester to manually enter the port number to access a webpage. (I performed this change manually on the web server version that is publicly accessible.)

smurthys commented 7 years ago

I agree with changing the web server's port to 80.

Don't need to do this but, we could find a way to optionally inform the server which port it should listen on using a startup parameter or config. This facility can be helpful in case port 80 has conflicts.

smurthys commented 7 years ago

Just a quick update that I have verified the proposed directory structure as well as the distributed deployment architecture works beautifully.

This is totally awesome team work, @DASSL/gradebook.

Here is a screenshot from my browser connecting to my local Gradebook server which connects to @afig's DBMS because that is what I asked the server to do at login.

So, I think the changes in this commit can be merged once the issues identified in this thread are addressed. After this merge, I will set up infrastructure for a reference deployment and establish a procedure for "nightly" builds.

image

smurthys commented 7 years ago

I believe the following changes are required before merging the changes in this PR: other changes/issues discussed are (may be) valid, but they can can be fixed against issues we can log after the merge with only the necessary changes.

wildtayne commented 7 years ago

I just pushed some commits to address the high priority issues we identified:

I will continue working on the other changes outlined in this discussion, but on a new branch.

afig commented 7 years ago

The latest changes that were implemented have been reflected in the web client. Also, the attendance data returned works perfectly with the client.

Another issue I have noticed is that there are some instances where giving incorrect parameters in a REST call causes the server to crash. Most of them cannot be triggered through the UI itself, so I believe we are okay for now if merging is a priority.