Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Move comments from browser storage to app #208

Open
jmcarp opened this issue Apr 7, 2016 · 4 comments
Open

Move comments from browser storage to app #208

jmcarp opened this issue Apr 7, 2016 · 4 comments

Comments

@jmcarp
Copy link
Contributor

jmcarp commented Apr 7, 2016

Why:

  • Simplify eventual support for non-js clients (more work required)
  • Allow eventual comment recovery across devices (more work required)

Steps:

  • Use anonymous sessions to track users
  • Drop browser storage in favor of app storage
  • Model per-section comments in ORM
  • Add CRUD endpoint for comments

For the comment data model, I'm thinking about something like:
Comment (user, section, label, text)
Attachment (name, size, key, commentid)

Although we could also store the attachment list as a blob, since we may not need to query it directly.

Questions:

  • Is this in scope for the pilot?
  • Is it important to support non-js clients?
  • Any security issues?

Note: this would replace the proposal to store comment text on s3 that was discussed yesterday.

@vrajmohan @tadhg-ohiggins @xtine @cmc333333

@vrajmohan
Copy link
Contributor

IIANM, the comment is currently stored as Markdown in localStorage. Why does moving it to the app necessitating more structured storage? Can't we get away with:
user, markdown_comment, attachment_list_blob.

I get it - just data transfer sizes across the network?

@cmc333333
Copy link
Member

Quick notes on implementation:

  • Django has a easy-to-use session system that stores to the database (or memcached, etc.)
  • I'd punt on using explicit ORM models; we can pull from the session store (which acts like a dict) when it's needed
  • -site currently assumes no database is present. There'd be some (minor) config changes to make that work. As a bonus, test configuration wouldn't be so confusing

Re importance/scope: I think this is a high second priority. We required LocalStorage + JS initially due to records management concerns. Had they not been an issue, we would have built with a progressive enhancement bent (starting with the assumption of hard reloads, building up the relevant AJAX). Unfortunately, that's not our situation -- though we're less concerned about records management now, we have additional work to support non-JS.

I'd argue that, our priority should remain building a full app with our initial platform (modern, JS browsers with LocalStorage). However, we know that we want to support non-JS at some point in the future, hopefully before the end of this pilot, so when we write code to satisfy our primary goal (the JS app), we should be engineering them towards the secondary goal (the non-JS app). From my perspective, we shouldn't work on this unless we have finished critical-path tasks.

@jmcarp
Copy link
Contributor Author

jmcarp commented Apr 7, 2016

we know that we want to support non-JS at some point in the future

Just curious--how do we know? Is this a regulatory requirement? Do many current users have js disabled?

we shouldn't work on this unless we have finished critical-path tasks

Agreed! What are those?

@cmc333333: are you saying that we want to continue using browser storage? Now I'm thinking I misunderstood our conversation on slack earlier today.

@cmc333333
Copy link
Member

Just curious--how do we know? Is this a regulatory requirement? Do many current users have js disabled?

Few webapps need JavaScript and there's nothing inherent to notice-and-comment that would appear to require it. Following the principles of progressive enhancement, webapps should provide a useable experience down to the point where the app is not longer useful. To give examples, if the webapp needs a slippy map, we need JS. If the webapp is just presenting content markup, there's zero reason to require JS.

What are those?

We chat about outstanding issues every few days, right? I'll post again in slack do prevent cluttering this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants