k-int / gokb-phase1

Original GOKb repo - Moving to https://github.com/openlibraryenvironment/gokb
http://www.gokb.org
Other
11 stars 5 forks source link

Cell level edit code invalid if captured while facet is applied #457

Closed kristenwilson closed 8 years ago

kristenwilson commented 8 years ago

I've just discovered that Refine capturing invalid code for cell level edits if the code is captured while a facet is applied. Can we see if this can be fixed quickly -- maybe with the 5.1 release? We're beginning to experiment with using this functionality, and 90% of the use cases for it seem to be with a facet applied. Thanks!

Here is code captured when no facet is applied. This works correctly if you reapply it. [ { "op": "gokb/capture-edit", "description": "Captured edit on cells in column DateLastPackageIssue based on 2 column values.", "engineConfig": { "mode": "row-based", "facets": [] }, "columnName": "DateLastPackageIssue", "expression": "if ( and (if( isNotNull(gokbCaseInsensitiveCellLookup('DateLastPackageIssue')), or (isBlank(cells[gokbCaseInsensitiveCellLookup('DateLastPackageIssue')]), isBlank(cells[gokbCaseInsensitiveCellLookup('DateLastPackageIssue')].value )), false), if( isNotNull(gokbCaseInsensitiveCellLookup('PublicationTitle')), and (isNotNull(cells[gokbCaseInsensitiveCellLookup('PublicationTitle')]), cells[gokbCaseInsensitiveCellLookup('PublicationTitle')].value == 'New Surveys in the Classics'), false)), '2012', value )", "onError": "keep-original", "repeat": false, "repeatCount": 0, "basedOn": [ "DateLastPackageIssue", "PublicationTitle" ], "value": "2012" } ]

Here is code captured for the same edit, but with a facet applied. Refine says this is invalid JSON if you try to reuse it.

[ { "op": "gokb/capture-edit", "description": "Captured edit on cells in column DateLastPackageIssue based on 2 column values.", "engineConfig": { "mode": "row-based", "facets": [ { "omitError": false, "expression": "if (isBlank(value), 'invalid', null)", "selectBlank": false, "selection": [ { "v": { "v": "invalid", "l": "invalid" } } ], "selectError": false, "invert": false, "name": "Invalid value in DateFirstPackageIssue", "omitBlank": true, "type": "list", "columnName": "DateFirstPackageIssue" } ]

ostephens commented 8 years ago

@kristenwilson can I check why the common scenario is to have a facet applied? Is this just to navigate to the line where the cell edit needs to take place, or is it to apply a condition which is key to the edit?

I'm just wondering because it feels like having a facet applied when reapplying a cell-level edit is counter to the idea of capturing the cell level edit - we could essentially end up capturing the same set of conditions twice through two different methods.

If the facet is just aiding the navigation to the correct line to do the cell edit (e.g. rather than paging through the file) then we might want to consider not capturing the facet when we capture the cell-level edit

kristenwilson commented 8 years ago

For me at least, the workflow tends to be:

  1. Use a facet to identify rows that need work (e.g., duplicate ISSNs)
  2. Look at a row and figure out what the change needs to be
  3. Apply the change and capture it
  4. Repeat for each row

To not have a facet applied, the workflow becomes a lot more cumbersome:

  1. Use a facet to identify rows that need work (e.g., duplicate ISSNs)
  2. Look at a row and figure out what the change needs to be
  3. Clear the facet
  4. Page to the affected row in the project
  5. Apply the change and capture it
  6. Reapply the facet
  7. Repeat for each row.

If you've got suggestions of ways to work around this without having to facet/unfacet between each change, the would be really helpful.

On Thu, Nov 26, 2015 at 4:47 AM, Owen Stephens notifications@github.com wrote:

@kristenwilson https://github.com/kristenwilson can I check why the common scenario is to have a facet applied? Is this just to navigate to the line where the cell edit needs to take place, or is it to apply a condition which is key to the edit?

I'm just wondering because it feels like having a facet applied when reapplying a cell-level edit is counter to the idea of capturing the cell level edit - we could essentially end up capturing the same set of conditions twice through two different methods.

If the facet is just aiding the navigation to the correct line to do the cell edit (e.g. rather than paging through the file) then we might want to consider not capturing the facet when we capture the cell-level edit

— Reply to this email directly or view it on GitHub https://github.com/k-int/gokb-phase1/issues/457#issuecomment-159860639.

Kristen Wilson Associate Head, Acquisitions and Discovery North Carolina State University Libraries 919-513-3354 kristen_wilson@ncsu.edu ORCID: http://orcid.org/0000-0002-3509-3417

ostephens commented 8 years ago

Hi @kristenwilson That workflow sounds about right to me but I think this definitely makes me thing that when we capture the cell level edits we should ignore any faceting or filtering that has been applied - when the cell level edit is repeated (which we can now do via a macro) this won't matter as all the conditions for the edit are captured in the cell level edit code.

@sosguthorpe does that make sense?

kristenwilson commented 8 years ago

@ostephens I agree with your suggestion about ignoring facets/filters when capturing the code for cell-level edits.

@sosguthorpe do you think we could roll this into the 5.1 release? It looks like that hasn't been deployed yet.

sosguthorpe commented 8 years ago

I'll take a look and see what's involved. Should be fine to roll it with the 5.1 release though yes.

sosguthorpe commented 8 years ago

@kristenwilson After trying (and failing) to replicate this issue I finally checked that the JSON you are copying is correct (something I should have done at the beginning I know...) and it is not. By not correct, I mean I checked it in JSONlint (jsonlint.com) and it is not valid JSON which leads me to believe that you are not copying the rule correctly and have missed the bottom half of the rule altogether. I have tested this and I have had no issues whatsoever.

Regarding the facets being captured, this is something that refine does natively (Try faceting and performing a text transformation), and something that I have reused here by calling the refine functionality. Considering nothing is actually broken I am hesitant to make any changes to this at all, as it behaves as intended and is consistent across refine.

kristenwilson commented 8 years ago

@sosguthorpe, you're exactly right. The "extract" display in Refine doesn't show the entire rule, and I was just not scrolling down to see the bottom half. (It cuts off at an awkward point.) Thanks for catching this.

So to understand correctly, code captured when a facet is applied will reapply that facet when it's used again? I'm not sure that's exactly what we want, but I agree we should work with the tool more before we make any additional changes.

So we should be ready to push out 5.1.