Editing records via TorqueDataConnect

[Nolski]

Overview

We would like to be able to edit the contents of submissions & store those edits separately from the original submissions. The main constraints are:

• Be able to edit in place, so basically when looking at the wiki page, be able to click around the data you want to change to change it
• Edits should survive re-runs of the ETL pipeline, and when the underlying data changes, users should be alerted to the fact that the edit may now be invalid

Then on top of that things that might be feature requests would be:

• ChangeLog of edits (that can be deleted, or modified)
• Ability to approve edits by admins

Further Context

Best guess for entry point it is replacing calls to "cull_invalid_columns" to something that culls and then puts in overlays int he torquedata python server. In that response, return not only the column value, but also some kind of html that javascript acts on.

Then, I would add a call to the python server of "get column X of proposal Y in sheet Z" for the popup, and another call of saving the edit.

[kfogel]

In our discussion today, we decided that the edit buttons would be controlled from the template, that the overlay table will live in memory (pace issue #26), that data writes will be atomic, and that these are the fields needed in the overlay table:

• Who made the edit
• Edit timestamp
• Which competition
• Which proposal
• Which column
• The original text (or a content hash thereof)
• The new text
• Log message (i.e., the reason for the edit) (defaults to NULL for now)
• Approval code
• Approver
• Approval timestamp

[kfogel]

As discussed in today's meeting, we will:

• Edit list-type items directly as newline-separated lists -- i.e., we'll expose the user to the internal representation in the spreadsheet. This is so that they can do anything: edit individual items, remove items, and add items. When in edit mode, we'll display a warning to the user informing them that the newline-based item separation needs to be preserved.

• We added the "log message" field in the overlay table (I've updated the previous comment accordingly). There will be no UI for this right now; we're just looking ahead. The field defaults to NULL.

[Nolski]

I think for the sake of our current implementation and communication on this ticket, I'm going to refrain from mentioning tables until we are committed to switching to a database. Currently our documents are stored in a dictionary formatted similarly to the following

data = {
    "proposals": {
        "1": {"Application #": "1", "Name": "foo"},
        "2": {"Application #": "2", "Name": "bar"}
    }
}

Our edit document will copy the above document when it is loaded and modify it to look like the following:

edits = {
    "proposals": {
        "1": [
            {
                "Application #": {
                    "new_value": "2",
                    "orig_value": "1", # or a hash?
                    "edit message": "",
                    "approver": "",
                    "edit_timestamp": "",
                    "approval_timestamp": "",
                },
                "Name": { ... },
            },
            { ...} # edits will be ordered in recency (most recent last)
        ],
        "2": { ... }
    }
}

[kfogel]

@Nolski, as we discussed, I created a branch in this repository for the editability feature work: 32-editability-feature. On that branch, I added the above data structure to the DESIGN.md file (see commit 3f36624e7dee).

I haven't ported over the changes from the torque-edit2 branch in your clone repository, because I figured you might want to squash/reorder things anyway. Feel free to to do so, including my commit above. It's a dev branch: any rebasing you want to do is fine. We only need things to remain stable once they're on the mainline or are on a designated stable branch (such as a release maintenance branch, for projects that are using those).

[kfogel]

Notes from our first preliminary walk-through of the feature with LFC:

• Need a “cancel” button as well as a “save” button, in case someone changes their mind about making the edit.
• List editability will include ability to add and delete items, as well as editing existing items, right? (See, e.g., the Cities list in Application A on the dev site.)
• Revertability: If we make an edit, we can roll it back? Do we need to design a UI for that?
• If we make an edit, API users will get that edit as the data they see, right?
• Logging: edits should be shown on the Special:Log page (which has recently been enhanced by us to include more stuff than Mediawiki natively logs; I don't think we ever had a ticket for that, but see for example commit 0f5e1357b7fe).

[Nolski]

If we make an edit, API users will get that edit as the data they see, right?

@kfogel This is currently not how it is set up as I assume mediawiki render endpoint is different from our API endpoint. Do we want it this way?

[frankduncan]

@Nolski The endpoint should be the same, but the format is different. We don't have a json format for the single object return, just mwiki. Edited records should show up under the sheet endpooint (/api/<sheet_name>.fmt), and also for the sheet_toc and search enpoint, in case the TOC or Search templates include edited columns.

[kfogel]

There definitely has to be one, consistent, up-to-date data set that people get, whether they come in via a wiki page or via API. That principle should hold for everything.

@frankduncan I couldn't tell from your response whether this principle will hold, currently. Will it?

[frankduncan]

@kfogel It should

[Nolski]

@kfogel it does :)

[Nolski]

Review notes here: https://github.com/OpenTechStrategies/torque/wiki/Meeting-Notes#2020-08-14-mikekarl-editability-feature-and-browser-compatibility

[kfogel]

@Nolski and @frankduncan, here's a list of all the editability issues we've discussed in recent check-in meetings (1, 2, 3). The purpose of this comment is to be a checklist we can go over to make sure we've covered every point:

• [x] Large text fields need correspondingly large/tall edit boxes.

  Also, if the editable text is going to extend below the bottom of the edit field, then the field needs a scrollbar.

• [x] Editability of fields that are next to the Snapshot box.

  Due to the effects of CSS float positioning, editing, say, the "Executive Summary" field at the top is wonky -- the edit field shows up way down below the snapshot box, because of how the box is floated. We need those fields to be editable too, though.

• [ ] Editing validated lists.

  E.g., in DemoView, see "Coding (subject, population, location)" with

  • Children and youth
  • Low-income people
  • People with disabilities

  as its list values. Those are pre-set values; in fact they get turned in to links. They can't just be changed to any arbitrary string via editing -- that would break things. So we should instead present a pre-validated list instead of a text-field.

• [x] What about editing fields that are mirrored in the Snapshot?

  We can't wait for a page refresh to get the snapshot updated -- it should update when the underlying data gets updated, that is, right when the "Save" button is clicked after editing.

• [x] The "duplicate fields on a page" problem.

  What happens if the same spreadsheet field (e.g., Description) is used in two different places on a proposal page, and someone edits one of those places? The other place will not be updated until the page is refreshed. But we don't necessarily want to refresh on every save-after-an-edit, because a person might have scrolled to get to the place where they are in the page (instead of having landed their directly via an anchored URL) in which case refreshing could confusingly reposition the page to somewhere unrelated to the edit they just made.

  We don't have to solve this to ship v1, but we should solve it eventually.

  (The solution may be related to whatever solution we have for the above item about editing fields that are mirrored in the Snapshot.)

• [x] Kicking the browser-compatibility can down the road for now.

  For now, manual testing in recent browsers will do. However, see issue #42 for some research Mike did into browser testing strategies and services.

• [x] Plugins can interfere with the editability feature.

  The Firefox Edit With Emacs plugin currently interferes with the Torque editability feature. If you have the plugin enabled, then clicking edit then clicking cancel will result in both the editable text field and the original rendered text being present, the former stacked above the latter, as described in an earlier meeting where we first discovered it.

  We need to figure out why the plugin has this effect, just in case the root cause has any wider implications for the robustness of the editability feature.

• [ ] Protect against the usual concurrent-edits race condition:

  1. X loads page P
  2. Y loads page P
  3. X starts editing field F on page P
  4. Y starts editing field F on page P
  5. Y saves her edits first
  6. X saves her edits, thus overwriting Y's changes

• [x] An edit can sometimes be lost, probably because JS call is asynchronous?

  Need to figure out what's going on and fix.

• [x] Text that we insert in the template could become part of an edit.

  Sometiems we insert text into a field directly in the template (e.g., indentation markers for spacing, or the word "CONFIDENTIAL:" or something like that. This text which does not come from the spreadsheet data itself should not be part of any edit.

  Mike and Frank discussed some possible solutions. They sounded reasonable to Karl, but Karl doesn't remember exactly what they were, so he sure hopes Mike and Frank do.

• [x] JS mis-minification bug.

  Because MediaWiki's JS minification doesn't handle the latest JS features, we simply mark our JS to not be minified. Let's just make sure that marking is done everywhere it needs to be done, and is documented (so that someone doesn't start minifying it later, mistakenly thinking that the lack of minification was merely an oversight rather than a deliberate decision).

• [x] Maybe improve the link between edit button and editable field.

  Mike and Frank are thinking about how best to programmatically (symbolically) link the edit button to the target field. There is a data-field attribute right now, but there might still be improvements possible. Karl doesn't know enough to fill in more details here, but hopes someone else does.

[kfogel]

@Nolski , what branch are your commits on? (Is it different from 32-editability-feature?)

I noticed that they all seem to have the same commit message, unless the GitHub UI is fooling me somehow. If your plan is to go back and give them more-specific commit messages later (and squash, reorder, whatever) as part of a giant rebase, that's fine -- I just want to know whether others should be looking at them yet.

We might want to make a convention for commits that are public-but-not-yet-intended-for-review, like "WIP/NR: " ("Work In Progress / Not Reviewable") or something like that.

[Nolski]

@kfogel apologies I keep my commits 1 per fix, feature, etc. That combined with having to commit any changes I want to test causes lots of amended commits and force pushes

[kfogel]

@Nolski No problem -- and no apology needed, that's all fine. I think it implicitly answers the two questions I asked, then:

1) You're on some other dev branch, not 32-editability-feature. 2) Others should not be looking at these commits yet.

If you're planning to amend a bunch of stuff later anyway, then there are a couple of routes we can go to avoid confusion in the issue: simply don't put the issue number in the commit messages right now (that way there won't be any noise in the issue, and you can add the issue number later when you're amending), or keep the issue number in and put a "WIP/NR: " prefix on the commit message, so others know that the noise just means "progress is happening" and not "please give feedback on this".

Of the two, I mildly lean toward the second way (and if you choose it, I'll document where our commit message conventions are documented), but either way is fine.

[Nolski]

@kfogel 32-editability-feature is the latest progress so I'll amend my latest commit to say wip that sounds like a good plan to me :)

[kfogel]

@Nolski:

Oh, oh oh oh OOOOOOH!

NOW I understand what's going on!

This issue shows a very long series of recent commits that all have the same commit message: "Resolves #32 by allowing edits to torque fields". I forgot that the GitHub UI doesn't do any kind of detection of replaced commits (i.e., of the typical kind of force push). That apparent series of commits is really all one commit, that you just keep amending.

When you said "I'll amend my latest commit" (singular), that's when the clue fell on me.

So, yes, this is fine: add the prefix and otherwise keep doing what you're doing and everything's good.