add identifiers to author edit page #5093

RayBB · 2021-04-25T06:10:46Z

Closes #853

I am addressing the PR in the minimum way following this comment.

To me, that means for this PR to be a success only the following need to be true:

A user can add a new identifier to an author
A user can modify an existing identifier on an author
A user can remove an existing identifier from an author

All three of these are possible using this minimal code.
Some possible future enhancements:

parsing a url to get identifier
validating identifiers
more listed in the original ticket

Enhancements with bigger scope:

dynamically render identifiers on author view page (right now they are hard coded)

Technical

From what I can see, this is one of the first implementation of Vue as an addition to an existing page. I'd really love feedback for how to make it better since I imagine others may be following this as an example.

I tried to keep things as simple as possible to meet the criteria.

For editions, the identifiers are setup as a list. However, for authors identifiers seem to be setup as dictionary. I'm not sure why this is but I figure for the scope of this PR I should just deal with it since I imagine doing a data conversion is not trivial.

I'm using template with the v-for because it allows me to have two root elements for each item in the list and that is need to use css grid.

Testing

Try adding and removing some identifiers from an author.

Setup

In order for this to work the appropriate config must be added to your environment by going to http://localhost:8080/config/author.yml?m=edit and setting:

identifiers:
-   label: ISNI
    name: isni
    notes: ''
    url: http://www.isni.org/@@@
    website: http://www.isni.org/
-   label: VIAF
    name: viaf
    notes: ''
    url: https://viaf.org/viaf/@@@
    website: https://viaf.org
-   label: Wikidata
    name: wikidata
    notes: ''
    url: https://tools.wmflabs.org/reasonator/?q=@@@
    website: https://wikidata.org
key: /config/author
type:
    key: /type/object

Screenshot

Screen.Recording.2021-05-04.at.8.42.27.PM.mov

Stakeholders

mekarpeles · 2021-04-27T20:18:32Z

@RayBB this is great that we're pushing the frontier on vue. This PR is useful as proof of concept even if we decide some changes may be required. Here's my first batch of opinionated feedback:

Especially for core features of the website (wiki, borrowing, reading log, lists, search) things should work with javascript disabled. This often makes a big difference when we're discussing website accessibility and this similarly also helps ensure under the hood we have very simple APIs which are GET/POST'able and can be automated for admin tasks. This doesn't mean we can't use vue, but I think this page in particular should probably work without it.
I think @jdlrobson may rightfully push for any of our js code to be outside of our templates and instead in /openlibrary/plugins/openlibrary/js/

jdlrobson · 2021-04-28T01:29:36Z

think @jdlrobson may rightfully push for any of our js code to be outside of our templates and instead in /openlibrary/plugins/openlibrary/js/

Yes please. Out of interest foes Python have any linting tools we could use to detect when that happens to assist volunteer contributors?

RayBB · 2021-04-28T02:34:22Z

As far as I can tell editing links on authors already requires JS and so does editing identifiers on editions (which this is based on). If the idea is that JS shouldn't be used for editing going lets add that to the Vue documentation and clarify where it is appropriate to use Vue.

It is possible to avoid JS all together if we require the users to save and reload the page after adding each identifier but I imagine that would feel quite cumbersome.

@jdlrobson It is possible to run a script that checks if any .vue or .js files were added outside of the /openlibrary/plugins/openlibrary/js/ . It could be a simple bash script but I don't know of any python specific linting tools that would do the job. If you make a separate issue goals for this I can look at it later.

Please let me know how to move forward. Since @cdrini initially setup the Vue work perhaps he has thought on this.

jdlrobson · 2021-04-28T03:13:30Z

As far as I can tell editing links on authors already requires JS and so does editing identifiers on editions (which this is based on). If the idea is that JS shouldn't be used for editing going lets add that to the Vue documentation and clarify where it is appropriate to use Vue.

I defer to Mek on this one but my belief about this is that without javascript it should be possible to read any page, search, and navigate to any page that can be rendered. Basic editing should be possible using a form submission, but its okay if it's a limited experience. For JavaScript enabled clients we can and should give the best experience possible. Using Vue.js on the editor seems perfectly sensible to me.

I do think we should adopt a storybook instance (#4179) and document our components before building too many things in Vue. That remains a high priority on my side for the frontend if we are going to maintain these Vue based components going forward. If we don't we may be destined to repeat the same mistakes of the past

@jdlrobson It is possible to run a script that checks if any .vue or .js files were added outside of the /openlibrary/plugins/openlibrary/js/ . It could be a simple bash script but I don't know of any python specific linting tools that would do the job. If you make a separate issue goals for this I can look at it later.

Sorry I should have looked at the pull request rather than done a drive by comment on my phone. Sorry about that.

This patch is fine Mek. We can and should use inline script tags inside Vue files. The only issue is inside python rendered templates. Sorry for the confusion here. #4474 captures that.

RayBB · 2021-05-04T04:25:16Z

@cdrini could you help me with the two things I wrote under Where I need help?

cdrini · 2021-05-04T19:43:48Z

Here's the editions' setup they're config: https://openlibrary.org/config/edition

Discussed on call:

Will need to create /config/authors (we'll copy it over to prod once it's created).
Fetching /config/authors from JS seems ok for now.
Not adding deletion right now seems reasonable, since there is already a pretty substantial contribution here.

RayBB · 2021-05-06T20:54:35Z

@cdrini If there is anything I should change let me know and I'll work on it this weekend.

cdrini

Nice work @RayBB ! A number of style/naming things, and a few suggestions on how to get the author_config working and make the logic a little simpler. Great work on testing the tires of the Vue flow!

openlibrary/components/AuthorIdentifiers.vue

openlibrary/plugins/upstream/utils.py

cdrini · 2021-05-06T22:11:47Z

Created the page here: https://openlibrary.org/config/author

Note I used a different link for Wikidata.

RayBB · 2021-05-13T05:46:14Z

@cdrini ready for another review. Thanks for the great comments!

These are the only two issues you raised that need a second look:
#5093 (comment)
#5093 (comment)

openlibrary/components/AuthorIdentifiers.vue

openlibrary/templates/type/author/edit.html

openlibrary/components/AuthorIdentifiers.vue

cdrini

Looking great! A few small final changes. I think those 3 vars will be clearer if we dissolve assignedIdentifiersWithConfigs ; that's the piece that's making this a little harder to read for me. Let me know if you have any questions!

openlibrary/components/AuthorIdentifiers.vue

cdrini · 2021-05-14T16:41:52Z

openlibrary/components/AuthorIdentifiers.vue

+        <button class="form-control" type="button" name="set" :disabled="!setButtonEnabled" @click=setIdentifier>Set</button>
+      </th>
+    </tr>
+    <tr :key="identifier.name" v-for="identifier in assignedIdentifiersWithConfigs">


Also add a v-if="value"; we don't want rows without identifiers to show up.

I don't think that we can do that using Vue 2 https://v3.vuejs.org/guide/migration/v-if-v-for.html#_2-x-syntax

Since this is only assigned identifiers, the only time rows with blank values will show up is if a user clicks remove.

That seems ok to me and lets us avoid more computed properties (the vue recommended solution).

The way they've written the docs there is super confusing :P But I think in Vue 3 this will no longer be possible; Vue 2 should support it: https://vuejs.org/v2/guide/list.html#v-for-with-v-if

Grrr, that's an annoying change in Vue 3 :/ Let's use it here for now, since we're still on Vue 2. I use this pattern profusely throughout library explorer 😅

RayBB · 2021-05-15T09:52:51Z

@cdrini your code reviews are amazing and I'm so impressed by how much you simplified this!

Ready for another review now

cdrini

Code lgtm! Two small fixes, and then I'll toss the latest code on testing for some... testing!

openlibrary/components/AuthorIdentifiers.vue

…tches from server and cleaned up

Co-authored-by: Drini Cami <cdrini@gmail.com>

…tches from server and cleaned up

Co-authored-by: Drini Cami <cdrini@gmail.com>

cdrini · 2021-05-17T22:03:31Z

Tested on testing:

✅ Adding a QID: http://testing.openlibrary.org/authors/OL23757A/Keith_J._Devlin?_compare=Compare&b=5&a=4&m=diff

Tested locally:

✅ Adding IDs + saving
✅ Removing previous ID
✅ Removing all identifiers - Doesn't seem to work; but can fix in a future issue/PR

mekarpeles assigned cdrini Apr 26, 2021

RayBB mentioned this pull request May 1, 2021

Adding multiple ocaid's in edit UI only adds one #395

Open

cdrini requested changes May 6, 2021

View reviewed changes

RayBB requested a review from cdrini May 13, 2021 05:45

cdrini reviewed May 14, 2021

View reviewed changes

cdrini requested changes May 14, 2021

View reviewed changes

cdrini reviewed May 14, 2021

View reviewed changes

openlibrary/components/AuthorIdentifiers.vue Outdated Show resolved Hide resolved

cdrini reviewed May 14, 2021

View reviewed changes

openlibrary/components/AuthorIdentifiers.vue Outdated Show resolved Hide resolved

cdrini reviewed May 14, 2021

View reviewed changes

cdrini approved these changes May 17, 2021

View reviewed changes

openlibrary/components/AuthorIdentifiers.vue Outdated Show resolved Hide resolved

openlibrary/components/AuthorIdentifiers.vue Outdated Show resolved Hide resolved

openlibrary/components/AuthorIdentifiers.vue Show resolved Hide resolved

cdrini reviewed May 17, 2021

View reviewed changes

openlibrary/components/AuthorIdentifiers.vue Outdated Show resolved Hide resolved

cdrini reviewed May 17, 2021

View reviewed changes

openlibrary/components/AuthorIdentifiers.vue Outdated Show resolved Hide resolved

RayBB added 8 commits May 17, 2021 16:47

add identifiers to author edit page

8bfa819

fix linter

3b79234

fix python linter

032055f

pass all remote ids in template, utils creates valid json, vue now fe…

2b486e0

…tches from server and cleaned up

add remove button, delete .page file

ded4a5f

remove extra lines

26a4db7

format code and add line to readme

9a868c4

remove aria invalid and move label to edit.html

b287584

RayBB and others added 23 commits May 17, 2021 16:47

output selector as prop

161f57a

selected -> selectedIdentifier

1d193ce

Co-authored-by: Drini Cami <cdrini@gmail.com>

remoteIdsParsed -> remoteIds

57aae7d

Co-authored-by: Drini Cami <cdrini@gmail.com>

rename selected -> selecteIdentifier and remoteIdsParsed -> remoteIds

d4b4fd0

use urllib.parse.quote

aafeee1

remove old comment

3e44c04

pass in identifiers instead of making a call

5bc71bf

simplify allIdentifiersByKey

1cf1b77

clearer variable names and docstrings

98faca9

use watch for assignedIdentifies

01c9a60

move div up for watch

a8b42c2

pass all remote ids in template, utils creates valid json, vue now fe…

a58977d

…tches from server and cleaned up

convert to html table

5441859

assignedIdentifiers watch deep set to true

a13f682

author_config -> author_config_string

770c049

remove label that was pointing to div

bd56087

Co-authored-by: Drini Cami <cdrini@gmail.com>

author_config_string in edit.html

a5187a9

use created instead of mounted

ee37e9e

remove type="button"

6db8e10

Co-authored-by: Drini Cami <cdrini@gmail.com>

remove type="button" from set button

32b2ad4

Co-authored-by: Drini Cami <cdrini@gmail.com>

dissolve assignedIdentifiersWithConfigs and better comments

8ca0fb8

comment on form-control

552ae7f

Small fixes from code review

fc7a989

cdrini force-pushed the 853/features/author-identifiers-ui branch from 1391baa to fc7a989 Compare May 17, 2021 22:04

cdrini merged commit d91dbad into internetarchive:master May 17, 2021

RayBB mentioned this pull request May 19, 2021

Translated from languages on edition edit page doesn't use setup_multi_input_autocomplete correctly #5193

Open

RayBB mentioned this pull request Jun 7, 2021

Adding two new identifiers #5275

Closed

jimchamp mentioned this pull request Jun 28, 2021

Add new remote ID links to author pages #5364

Merged

RayBB mentioned this pull request Oct 7, 2021

Add author config to local instance automatically #5742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add identifiers to author edit page #5093

add identifiers to author edit page #5093

add identifiers to author edit page #5093

add identifiers to author edit page #5093

Conversation

Technical

Testing

Setup

Screenshot

Stakeholders

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment