Re-enable extending edition dropdowns as librarian only #2700

cdrini · 2019-12-06T23:04:04Z

Feature? At some point the option to create new roles, identifiers, or classifications was disabled. Bringing it back as a librarian feature.

The question is: Do we even want this? Or is it better to axe this feature (#2496)? This code has been dead for a loooong time. If we really need to, it looks like we can extend these select dropdowns by editing e.g. https://openlibrary.org/config/edition anyways.

Technical

It seems like edits which create a new role appear as "Adminstrator" edits on local?

Testing

Haven't done any "real" testing

Got to e.g. http://localhost:8080/books/OL2058361M/The_wit_wisdom_of_Mark_Twain/edit
Select "Add new" in the role/identifier/classifications select dropdown

Evidence

Stakeholders

@jdlrobson @hornc @seabelis @tfmorris

cdrini · 2019-12-06T23:04:41Z

@seabelis How useful would this feature be as a librarian? I know there was an issue recently about modifying the items in this list.

cdrini · 2019-12-10T20:13:28Z

@mekarpeles @hornc @seabelis @cdrini decided on call that this will be merged

At some point the option to create new roles, identifiers, or classifications was disabled. Bringing it back as a librarian feature.

The JS had some random bugs/issues; potentially jquery update related? Or potentially always there. Not sure.

Librarian only feature, and librarians _should_ have permissions.

jimchamp · 2021-07-13T00:07:18Z

openlibrary/templates/books/edit/edition.html

-                                    <!-- <option>---</option> -->
-                                    <!-- <option value="__add__">$_("Add a new role")</option> -->
+                                    $if ctx.user and (ctx.user.is_admin() or ctx.user.is_librarian()):
+                                        <option>--- $_("Librarian Only")</option>


Suggested change

<option>--- $_("Librarian Only")</option>

<option disabled>--- $_("Librarian Only")</option>

Prevents "--- Librarian Only" from being selected accidentally (probably not much of a concern).

jimchamp · 2021-07-13T00:09:16Z

openlibrary/templates/books/edit/edition.html

@@ -208,8 +208,9 @@
                                    $for role in edition_config.roles:
                                        <option>$role</option>

-                                    <!-- <option>---</option> -->
-                                    <!-- <option value="__add__">$_("Add a new role")</option> -->
+                                    $if ctx.user and (ctx.user.is_admin() or ctx.user.is_librarian()):


Suggested change

$if ctx.user and (ctx.user.is_admin() or ctx.user.is_librarian()):

$if is_privileged_user:

openlibrary/openlibrary/templates/books/edit/edition.html

Line 4 in a9d82b6

$ is_privileged_user = ctx.user and (ctx.user.is_admin() or ctx.user.is_librarian())

This will DRY things up.

jimchamp · 2021-07-13T00:14:30Z

openlibrary/plugins/openlibrary/js/add_new_field.js

+    }
+
+    function error(error_div, input, message) {
+        $(error_div).show().html(message);


This isn't displaying the error message divs. These divs have classes hidden and popalert, which are both have display: none.

If the message were displayed, it would probably cause the content div to expand past the modal container:

It may be easier to display the error message to the right of the text input.

jimchamp

The code seems to be working properly. My notes about disabling the "--- Librarians Only" option and the error message not being displayed may not be too big of an issue, as not all patrons will be interacting with this feature.

jimchamp · 2021-07-13T00:25:56Z

openlibrary/plugins/openlibrary/js/add_new_field.js

+    }
+
+    function error(error_div, input, message) {
+        $(error_div).show().html(message);


If the message were displayed, it would probably cause the content div to expand past the modal container:

It may be easier to display the error message to the right of the text input.

jimchamp · 2021-07-13T00:26:32Z

openlibrary/templates/books/edit/edition.html

+                                $if ctx.user and (ctx.user.is_admin() or ctx.user.is_librarian()):
+                                    <option>--- $_("Librarian Only")</option>


Same notes as above.

jimchamp · 2021-07-13T00:26:53Z

openlibrary/templates/books/edit/edition.html

+                                $if ctx.user and (ctx.user.is_admin() or ctx.user.is_librarian()):
+                                    <option>--- $_("Librarian Only")</option>


Again, same notes as above.

bicolino34 · 2022-10-17T07:42:50Z

@cdrini is this still work in progress?

cdrini added Needs: Feedback A proposed feature or bug resolution needs community feedback prior to forging ahead. [managed] State: Work In Progress This issue is being actively worked on. [managed] labels Dec 6, 2019

cdrini mentioned this pull request Dec 6, 2019

Remove add_new_field to role/classifications/identifier dead code #2496

Closed

mekarpeles removed the State: Work In Progress This issue is being actively worked on. [managed] label Apr 10, 2020

mekarpeles marked this pull request as draft April 10, 2020 23:23

mekarpeles self-assigned this Apr 10, 2020

mekarpeles changed the title ~~WIP: Re-enable extending edition dropdowns as librarian only~~ Re-enable extending edition dropdowns as librarian only Apr 10, 2020

cdrini unassigned mekarpeles Dec 14, 2020

Re-enable extending edition dropdowns as librarian only

17a9351

At some point the option to create new roles, identifiers, or classifications was disabled. Bringing it back as a librarian feature.

cdrini force-pushed the feature/expand-edition-dropdowns branch from 56bdf8e to 97bf252 Compare June 19, 2021 03:17

cdrini added 3 commits June 18, 2021 23:22

Migrate to js-land & re-animate JS from addfield.html

2e12590

The JS had some random bugs/issues; potentially jquery update related? Or potentially always there. Not sure.

Don't elevate to admin to edit /config/edition

b338985

Librarian only feature, and librarians _should_ have permissions.

Improve markup around librarian only add option

a9d82b6

cdrini force-pushed the feature/expand-edition-dropdowns branch from 97bf252 to a9d82b6 Compare June 19, 2021 03:22

cdrini removed the Needs: Feedback A proposed feature or bug resolution needs community feedback prior to forging ahead. [managed] label Jun 19, 2021

cdrini marked this pull request as ready for review June 19, 2021 03:27

cdrini assigned jimchamp Jun 21, 2021

jdlrobson mentioned this pull request Jun 25, 2021

EPIC: Remove inline JS across all python templates #4474

Open

40 tasks

jimchamp reviewed Jul 13, 2021

View reviewed changes

jimchamp requested changes Jul 13, 2021

View reviewed changes

lephemere mentioned this pull request Jul 21, 2021

Refactor/Move inline js from books/edit/addfield template #5138

Closed

jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Aug 30, 2021

mekarpeles added the Priority: 2 Important, as time permits. [managed] label Sep 27, 2021

cdrini added Priority: 3 Issues that we can consider at our leisure. [managed] and removed Priority: 2 Important, as time permits. [managed] labels Oct 18, 2021

cdrini marked this pull request as draft October 19, 2023 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable extending edition dropdowns as librarian only #2700

Re-enable extending edition dropdowns as librarian only #2700

cdrini commented Dec 6, 2019

cdrini commented Dec 6, 2019

cdrini commented Dec 10, 2019

jimchamp Jul 13, 2021

jimchamp Jul 13, 2021

jimchamp Jul 13, 2021

jimchamp Jul 13, 2021

jimchamp left a comment

jimchamp Jul 13, 2021

jimchamp Jul 13, 2021

jimchamp Jul 13, 2021

bicolino34 commented Oct 17, 2022

	<option>--- $_("Librarian Only")</option>
	<option disabled>--- $_("Librarian Only")</option>

	$if ctx.user and (ctx.user.is_admin() or ctx.user.is_librarian()):
	$if is_privileged_user:

Re-enable extending edition dropdowns as librarian only #2700

Are you sure you want to change the base?

Re-enable extending edition dropdowns as librarian only #2700

Conversation

cdrini commented Dec 6, 2019

Technical

Testing

Evidence

Stakeholders

cdrini commented Dec 6, 2019

cdrini commented Dec 10, 2019

jimchamp Jul 13, 2021

Choose a reason for hiding this comment

jimchamp Jul 13, 2021

Choose a reason for hiding this comment

jimchamp Jul 13, 2021

Choose a reason for hiding this comment

jimchamp Jul 13, 2021

Choose a reason for hiding this comment

jimchamp left a comment

Choose a reason for hiding this comment

jimchamp Jul 13, 2021

Choose a reason for hiding this comment

jimchamp Jul 13, 2021

Choose a reason for hiding this comment

jimchamp Jul 13, 2021

Choose a reason for hiding this comment

bicolino34 commented Oct 17, 2022