Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ATLAS-178: Support upload of marker images #95

Merged
merged 1 commit into from Jul 15, 2019

Conversation

@HelioStrike
Copy link
Collaborator

commented Jul 12, 2019

JIRA issue: https://issues.openmrs.org/browse/ATLAS-178

The images are being stored on the MySQL server as a BLOB.

(I'm still looking for the CSS issue that's causing the image to float to the left, instead having its own row)
Screenshot from 2019-07-12 18-10-35
Screenshot from 2019-07-12 18-09-44
Screenshot from 2019-07-12 18-20-23
Screenshot from 2019-07-12 18-20-31

@HelioStrike HelioStrike requested review from bmamlin and cintiadr Jul 12, 2019

@HelioStrike HelioStrike changed the title ATLAS=178: Support upload of marker images ATLAS-178: Support upload of marker images Jul 12, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

@bmamlin @cintiadr I just realized that the way I changed the database may be troublesome for the RSS feed if we'd like to display image links. I think we'll need an API to retrieve images to keep everything consistent. What do you think? If the API idea seems fine, then how to we structure the url?

@bmamlin
Copy link
Member

left a comment

Great job! I've ran into a few issues testing this and I believe they are related:

When I tried uploading an image that's ~119 KB, I got an error message that looked like a 413 Payload Too Large response... which I had already suggested adding in the API. Since I'm seeing it and for an image below the max size, I'm assuming node or express is enforcing a maximum and responding accordingly. We should find out where that's coming from so we know if/how we can control it.

When I upload an image, I have to reload the page before it shows in the marker.

The biggest change I think we need is more significant. Storing the image in the database is fine (especially with limiting the size they can be); however, I do not like storing the image data directly within the marker resource for a few reasons:

  • Working with marker resources that have images becomes much messier (unnecessary bloat in log entries, debugging, etc.)
  • The marker resource becomes unnecessarily large (the 413 Payload Too Large error I'm getting might be a symptom of that)
  • Most importantly, it doesn't give us an easy way to cache images (i.e., we'd like the option to have nginx cache these images so they can be loaded most times without hitting the API or database).

To address this, I would suggest designing the marker resource to have a read-only image_url property that the API sets to a location of the image. You can still store the image as a blob in the atlas table, but, when returning a marker, instead of an image it would have an image_url like {server_address}/marker/{id}/image.

If we can get around the payload size issues of node/express, then you could allow a marker to be POSTed with the image data in an image property, but it would return without image and an image_url instead. Otherwise (or in addition), you images could be managed with GET, POST, PUT, and DELETE calls to /marker/{id}/image.

routes/markers.js Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
db/bootstrap.sql Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 13, 2019

@bmamlin After an hour of searching, I finally figured out how to send data strings as images. -_-;

While fixing up the remaining code to use the API, I feel like it would be easier to have a new table for images (say atlas_images(id, datastring)), or separate columns for image-data and image-url. It would make the code more readable, and we won't need to alter the image column manually everytime a GET is being performed. Additionally, markers currently using external links won't be affected. What do you think of this?

The PR for the initial plan (having a single image column in the atlas table) is almost complete, so I'm fine with either. :)

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-178 branch 4 times, most recently from 7db6a8f to 6de3314 Jul 13, 2019

@HelioStrike HelioStrike requested a review from bmamlin Jul 13, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 13, 2019

@bmamlin I made the changes as requested :)

If you think the separate column idea is ok, I'll make the changes in this PR. :)

@bmamlin
Copy link
Member

left a comment

Changes look great!

You could avoid unnecessarily getting all the images from the database & having to manually insert urls by replacing image in your sql SELECT statement with something like:

IF(image, concat(?,'://',?,'/marker/',id,'/image'), null) AS image_url,

supplying req.protocol and req.headers.host to the prepared statement.

Otherwise, I suggested a small styling change to the image size tip. If you can apply those changes, we can merge this PR. We'll need to add a mechanism for image removal... but we could add that in a subsequent ticket.

public/js/user.js Show resolved Hide resolved

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-178 branch from 6de3314 to 75dded5 Jul 15, 2019

@HelioStrike HelioStrike requested a review from bmamlin Jul 15, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

@bmamlin I made the changes :)

IF(image, concat(?,'://',?,'/marker/',id,'/image'), null) AS image_url,

is an amazing idea, and completely eliminates the need for a new column! I had to tweak it to

IF(image is not null, concat(?,'://',?,'/marker/',id,'/image'), null) AS image_url,

Its weird why the former doesn't work, but I thought I'd report the observation, as its something I may probably waste a lot of time on in the future if I didn't remember it. :p

While testing, please also take a look at the RSS feed, and the marker csv, as I made a few tiny changes to them while getting this PR working. :)

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-178 branch from 75dded5 to ef79f3f Jul 15, 2019

@bmamlin
Copy link
Member

left a comment

Almost there! A few loose ends to cut and this should be ready to merge!

public/js/user.js Outdated Show resolved Hide resolved
app.js Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
routes/markers.js Outdated Show resolved Hide resolved

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-178 branch from ef79f3f to c2c70a8 Jul 15, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-178 branch from c2c70a8 to b9bc88f Jul 15, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

How did this get closed? ._.

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

Got closed accidentally, after I pushed without committing. Also, made the changes. :)

@HelioStrike HelioStrike reopened this Jul 15, 2019

@HelioStrike HelioStrike requested a review from bmamlin Jul 15, 2019

@bmamlin bmamlin merged commit 098ebee into openmrs:master Jul 15, 2019

@HelioStrike HelioStrike deleted the HelioStrike:ATLAS-178 branch Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.