Extract view/link URL for reconciled cells #1457

[ayushrai206]

Fixes #1457

Changes proposed in this pull request:

• This basically adds another option of adding a column with url of matched entities in the reconcile->actions menu.

[wetneb]

Really great work, congratulations!

We will likely need to improve the GREL expression a little so that it works in all cases.
For Wikidata, the identifiers of entities are of the form Q3498, so they can be directly inserted in the template and that gives a valid URL.

But some reconciliation services might use identifiers which include some characters which need to be escaped to make sure we get a valid URL.
The specification has a note about this:

To guarantee the correctness of the formed URI, clients MUST percent-encode component-delimiting reserved characters in the identifier, i.e. encode it as a URI component [RFC3986].

So we need to find a GREL function which can be used to do this encoding and then insert it at the right place in the expression to get the desired result.

[wetneb]

Your last commit 55010a6 is likely unintentional, no? The menu-reconcile.js file should not be deleted.

[ayushrai206]

Your last commit 55010a6 is likely unintentional, no? The menu-reconcile.js file should not be deleted.

yes that was by mistake!

[wetneb]

Congratulations @ayushrai206, it's looking great. However it looks like you have added some files by mistake (see the "Files changed" tab of this PR).

[wetneb]

We can now do some style polishing, see the comments below.
Also, the file Add column_with_URLs_of_matched_entities.cy.js could be renamed to add_column_with_URLs_of_matched_entities.cy.js (to avoid having a space in its filename, and to follow the naming convention of other files in that directory).

[wetneb]

Almost there! Just a couple of nitpicks about code style.

[wetneb]

Good start! It looks like there are some issues with the overall structure of your code though.

[ayushrai206]

Hey everyone,
In my most recent commit I have tried to resolve the problem of infinite loop and I have also commented out the cypress tests for a while since they were giving errors, will update the cypress tests when the code is approved. Also I was unsuccessful in removing force pushes from Github history so please do not compare the code in the force pushes, rather see my most recent commit for the actual changes.
Thank you
Ayushi

[wetneb]

Congratulations, it seems to work great!

It is a little difficult to review your code because you did not add any indentation. Could you indent it like the surrounding code, by using two spaces as indentation unit?

Also, the purpose of introducing this promptForColumn was to avoid the duplication of code with the "Add entity identifiers column" operation. So we should make sure that operation also switches to using this new function you introduced. You might need to add another parameter to make it possible to set the title of the dialog (since the current dialog title mentions URLs, which will not be appropriate for the other operation).

[wetneb]

Thanks, that's much better! It seems that by doing this refactoring you have changed the title of the dialog of the existing operation. Restoring the original title should help to get the Cypress test pass again. I would do that by adding a new argument to the promptForColumn function, which would be the title to display in the dialog. This way, you would still be able to use the same promptForColumn function in both operations, but display different titles to the user. This would let you keep the original dialog title for the existing operation and use an appropriate title (mentioning URLs) for the new operation.

[wetneb]

Great, the Cypress tests pass! Now perhaps you could re-enable the Cypress test for the new operation and we should be good to go :) I think the indentation can be improved still but I'm happy to do that at the end.

[wetneb]

The indentation can still be improved here. You can look for other calls to Refine.postCoreProcess for inspiration :)