From 6fe9d7d7a68a81a75a3ffce3b99d5aa3181cf96b Mon Sep 17 00:00:00 2001 From: Joshua Wehner Date: Fri, 15 Sep 2023 14:12:41 +0200 Subject: [PATCH] Allow org admins to add members as co-authors (#20008) * Basics might be working? * Stop propagating button clicks in autocomplete pills * Better blank slate * Better location to stop propagation * Remove author_id from org co-authors * Move UserStore and try testing it * Remove extraneous comments * OH... that's what that does! * Very basic testing * Re-organize javascripts * Rename & re-org for testing * Cleanup * More tests * Remove unnecessary nesting * Coninuing to try to bump coverage * Include /packs/ in code coverage metric * Try tweaking jest coverage more? We probably can't collect coverage from all of packs/* (because coverage is too low) but maybe we can try to opt-in for newer areas as we go? * Relocate JS tests, for build & coverage * User ID exception on search, not fetch * Remove commented-out console.log --------- Co-authored-by: Mac Siri --- app/controllers/articles_controller.rb | 3 +- app/controllers/organizations_controller.rb | 5 + ...rtCoauthorIdsToUsernameInputs.test.js.snap | 57 ++++++ .../{packs => }/__tests__/billboard.test.js | 2 +- ...convertCoauthorIdsToUsernameInputs.test.js | 162 ++++++++++++++++++ .../MultiSelectAutocomplete.jsx | 7 +- .../convertCoauthorIdsToUsernameInputs.js | 49 ++++++ app/javascript/shared/components/UserStore.js | 59 +++++++ .../shared/components/UsernameInput.jsx | 49 ++++++ .../components/__tests__/UserStore.test.js | 79 +++++++++ .../__tests__/UsernameInput.test.js | 49 ++++++ .../__snapshots__/UsernameInput.test.js.snap | 3 + app/javascript/utilities/document_ready.js | 7 + app/models/article.rb | 4 + app/services/articles/attributes.rb | 2 +- .../_dashboard_article_row.html.erb | 15 ++ app/views/dashboards/show.html.erb | 1 + config/locales/views/dashboard/en.yml | 1 + config/locales/views/dashboard/fr.yml | 1 + jest.config.js | 1 + 20 files changed, 552 insertions(+), 4 deletions(-) create mode 100644 app/javascript/__tests__/__snapshots__/convertCoauthorIdsToUsernameInputs.test.js.snap rename app/javascript/{packs => }/__tests__/billboard.test.js (93%) create mode 100644 app/javascript/__tests__/convertCoauthorIdsToUsernameInputs.test.js create mode 100644 app/javascript/packs/dashboards/convertCoauthorIdsToUsernameInputs.js create mode 100644 app/javascript/shared/components/UserStore.js create mode 100644 app/javascript/shared/components/UsernameInput.jsx create mode 100644 app/javascript/shared/components/__tests__/UserStore.test.js create mode 100644 app/javascript/shared/components/__tests__/UsernameInput.test.js create mode 100644 app/javascript/shared/components/__tests__/__snapshots__/UsernameInput.test.js.snap create mode 100644 app/javascript/utilities/document_ready.js diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 28bb9d3b4..b96acd481 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -328,8 +328,9 @@ class ArticlesController < ApplicationController # NOTE: the organization logic is still a little counter intuitive but this should # fix the bug - if params["article"]["user_id"] && org_admin_user_change_privilege + if org_admin_user_change_privilege allowed_params << :user_id + allowed_params << :co_author_ids_list elsif params["article"]["organization_id"] && allowed_to_change_org_id? # change the organization of the article only if explicitly asked to do so allowed_params << :organization_id diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index ee5937b9d..4a2b91902 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -97,6 +97,11 @@ class OrganizationsController < ApplicationController def members @organization = Organization.find_by(slug: params[:slug]) @members = @organization.users + + respond_to do |format| + format.json { render json: @members.to_json(only: %i[id name username]) } + format.html + end end private diff --git a/app/javascript/__tests__/__snapshots__/convertCoauthorIdsToUsernameInputs.test.js.snap b/app/javascript/__tests__/__snapshots__/convertCoauthorIdsToUsernameInputs.test.js.snap new file mode 100644 index 000000000..c1f618830 --- /dev/null +++ b/app/javascript/__tests__/__snapshots__/convertCoauthorIdsToUsernameInputs.test.js.snap @@ -0,0 +1,57 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`convertCoauthorIdsToUsernameInputs when there *is* a pre-existing id value renders matching snapshot 1`] = ` +" +
+ + +
+ +
+ +
Maximum 4 selections

Selected items:

    + +
    +
    + + + " +`; + +exports[`convertCoauthorIdsToUsernameInputs when there is no pre-existing id value renders matching snapshot 1`] = ` +" +
    + + +
    + +
    + +
    Maximum 4 selections

    Selected items:

      + +
      +
      + + + " +`; diff --git a/app/javascript/packs/__tests__/billboard.test.js b/app/javascript/__tests__/billboard.test.js similarity index 93% rename from app/javascript/packs/__tests__/billboard.test.js rename to app/javascript/__tests__/billboard.test.js index e77a695fe..b5ee4dade 100644 --- a/app/javascript/packs/__tests__/billboard.test.js +++ b/app/javascript/__tests__/billboard.test.js @@ -1,4 +1,4 @@ -import { getBillboard } from '../billboard'; +import { getBillboard } from '../packs/billboard'; describe('getBillboard', () => { beforeEach(() => { diff --git a/app/javascript/__tests__/convertCoauthorIdsToUsernameInputs.test.js b/app/javascript/__tests__/convertCoauthorIdsToUsernameInputs.test.js new file mode 100644 index 000000000..2723bde77 --- /dev/null +++ b/app/javascript/__tests__/convertCoauthorIdsToUsernameInputs.test.js @@ -0,0 +1,162 @@ +import fetch from 'jest-fetch-mock'; +import '@testing-library/jest-dom'; +import userEvent from '@testing-library/user-event'; + +import { convertCoauthorIdsToUsernameInputs } from '../packs/dashboards/convertCoauthorIdsToUsernameInputs'; + +global.fetch = fetch; + +function fakeFetchResponseJSON() { + return JSON.stringify([ + { name: 'Alice', username: 'alice', id: 1 }, + { name: 'Bob', username: 'bob', id: 2 }, + { name: 'Charlie', username: 'charlie', id: 3 }, + ]); +} + +describe('convertCoauthorIdsToUsernameInputs', () => { + beforeEach(() => { + global.Honeybadger = { notify: jest.fn() }; + + fetch.resetMocks(); + fetch.mockResponse(fakeFetchResponseJSON()); + }); + + describe('when there is no pre-existing id value', () => { + beforeEach(() => { + window.document.body.innerHTML = ` +
      +
      + + +
      + +
      + +
      + +
      +
      + + +
      + `; + }); + + it('calls fetch, with exception for author ID', async () => { + await convertCoauthorIdsToUsernameInputs(); + expect(fetch).toHaveBeenCalledWith('/org7053/members.json'); + }); + + it('makes the co-author field hidden', async () => { + await convertCoauthorIdsToUsernameInputs(); + const co_author_field = document.getElementById( + 'article_ID_co_author_ids_list', + ); + expect(co_author_field.type).toBe('hidden'); + }); + + it('renders matching snapshot', async () => { + await convertCoauthorIdsToUsernameInputs(); + expect(document.forms[0].innerHTML).toMatchSnapshot(); + }); + + it('works as expected', async () => { + await convertCoauthorIdsToUsernameInputs(); + const input = document.querySelector( + "input[placeholder='Add up to 4...']", + ); + input.focus(); + await userEvent.type(input, 'Bob,'); + + const hiddenField = document.querySelector( + "input[name='article[co_author_ids_list]']", + ); + expect(hiddenField.value).toBe('2'); + }); + }); + + describe('when there *is* a pre-existing id value', () => { + beforeEach(() => { + window.document.body.innerHTML = ` +
      +
      + + +
      + +
      + +
      + +
      +
      + + +
      + `; + }); + + it('renders matching snapshot', async () => { + await convertCoauthorIdsToUsernameInputs(); + expect(document.forms[0].innerHTML).toMatchSnapshot(); + }); + + it('works as expected', async () => { + await convertCoauthorIdsToUsernameInputs(); + const input = document.querySelector( + "input[placeholder='Add another...']", + ); + input.focus(); + await userEvent.type(input, 'Bob,'); + + const hiddenField = document.querySelector( + "input[name='article[co_author_ids_list]']", + ); + expect(hiddenField.value).toBe('3, 2'); + }); + + it('can remove previously selected', async () => { + await convertCoauthorIdsToUsernameInputs(); + const deselect = document.querySelector( + '.c-autocomplete--multi__selected', + ); + await userEvent.click(deselect); + + const hiddenField = document.querySelector( + "input[name='article[co_author_ids_list]']", + ); + expect(hiddenField.value).toBe(''); + }); + }); +}); diff --git a/app/javascript/crayons/MultiSelectAutocomplete/MultiSelectAutocomplete.jsx b/app/javascript/crayons/MultiSelectAutocomplete/MultiSelectAutocomplete.jsx index 25d738fdd..959b813f2 100644 --- a/app/javascript/crayons/MultiSelectAutocomplete/MultiSelectAutocomplete.jsx +++ b/app/javascript/crayons/MultiSelectAutocomplete/MultiSelectAutocomplete.jsx @@ -598,7 +598,12 @@ export const MultiSelectAutocomplete = ({ className={`c-autocomplete--multi__wrapper${ border ? '-border crayons-textfield' : ' border-none p-0' } flex items-center cursor-text`} - onClick={() => inputRef.current?.focus()} + onClick={(event) => { + // Stopping propagation here so that clicks on the 'x' close button + // don't appear to be "outside" of any container (eg, dropdown) + event.stopPropagation(); + inputRef.current?.focus(); + }} >
        {allSelectedItemElements} diff --git a/app/javascript/packs/dashboards/convertCoauthorIdsToUsernameInputs.js b/app/javascript/packs/dashboards/convertCoauthorIdsToUsernameInputs.js new file mode 100644 index 000000000..173725d4c --- /dev/null +++ b/app/javascript/packs/dashboards/convertCoauthorIdsToUsernameInputs.js @@ -0,0 +1,49 @@ +import { h, render } from 'preact'; +import { UsernameInput } from '../../shared/components/UsernameInput'; +import { UserStore } from '../../shared/components/UserStore'; +import '@utilities/document_ready'; + +export async function convertCoauthorIdsToUsernameInputs() { + const usernameFields = document.getElementsByClassName( + 'article_org_co_author_ids_list', + ); + + const users = new UserStore(); + + for (const targetField of usernameFields) { + targetField.type = 'hidden'; + const exceptAuthorId = + targetField.form.querySelector('#article_user_id').value; + const inputId = `auto${targetField.id}`; + const fetchUrl = targetField.dataset.fetchUsers; + const row = targetField.parentElement; + + await users.fetch(fetchUrl).then(() => { + const value = users.matchingIds(targetField.value.split(',')); + const fetchSuggestions = function (term) { + return users.search(term, { except: exceptAuthorId }); + }; + + const handleSelectionsChanged = function (ids) { + targetField.value = ids; + }; + + render( + , + row, + ); + }); + } +} + +document.ready.then(() => { + convertCoauthorIdsToUsernameInputs(); +}); diff --git a/app/javascript/shared/components/UserStore.js b/app/javascript/shared/components/UserStore.js new file mode 100644 index 000000000..088fe30a3 --- /dev/null +++ b/app/javascript/shared/components/UserStore.js @@ -0,0 +1,59 @@ +export class UserStore { + constructor() { + this.users = []; + this.wasFetched = false; + } + + fetch(url) { + const myStore = this; + return new Promise((resolve, _reject) => { + if (myStore.wasFetched) { + resolve(); + } else { + window + .fetch(url) + .then((res) => res.json()) + .then((data) => { + myStore.users = data.reduce((array, aUser) => { + array.push(aUser); + return array; + }, []); + myStore.wasFetched = true; + resolve(); + }) + .catch((error) => { + Honeybadger.notify(error); + resolve(); + }); + } + }); + } + + matchingIds(arrayOfIds) { + const allUsers = this.users; + const someUsers = arrayOfIds.reduce((array, idString) => { + const aUser = allUsers.find((user) => user.id == idString); + if (typeof aUser != 'undefined') { + array.push(aUser); + } + return array; + }, []); + return someUsers; + } + + search(term, options) { + options ||= {}; + const { except } = options; + const allUsers = this.users; + const results = []; + for (const aUser of allUsers) { + if ( + aUser.id != except && + (aUser.name.search(term) >= 0 || aUser.username.search(term) >= 0) + ) { + results.push(aUser); + } + } + return results; + } +} diff --git a/app/javascript/shared/components/UsernameInput.jsx b/app/javascript/shared/components/UsernameInput.jsx new file mode 100644 index 000000000..b22fbf34c --- /dev/null +++ b/app/javascript/shared/components/UsernameInput.jsx @@ -0,0 +1,49 @@ +import { h } from 'preact'; +import PropTypes from 'prop-types'; +import { MultiSelectAutocomplete } from '@crayons/MultiSelectAutocomplete/MultiSelectAutocomplete'; + +/** + * UsernameInput — produces a field that can autocomplete usernames + * + * @param {Function} fetchSuggestions Callback to sync selections to article form state + * @param {string} defaultValue Comma separated list of any currently user IDs + */ +export const UsernameInput = ({ + fetchSuggestions, + defaultValue, + inputId, + labelText, + placeholder, + maxSelections, + handleSelectionsChanged, +}) => { + const onSelectionsChanged = function (selections) { + const ids = selections.map((item) => item.id).join(', '); + handleSelectionsChanged?.(ids); + }; + + return ( + + ); +}; + +UsernameInput.propTypes = { + fetchSuggestions: PropTypes.func.isRequired, + defaultValue: PropTypes.string, + inputId: PropTypes.string, + labelText: PropTypes.string, + placeholder: PropTypes.string, + maxSelections: PropTypes.string, + handleSelectionsChanged: PropTypes.func.isRequired, +}; diff --git a/app/javascript/shared/components/__tests__/UserStore.test.js b/app/javascript/shared/components/__tests__/UserStore.test.js new file mode 100644 index 000000000..a71cada7c --- /dev/null +++ b/app/javascript/shared/components/__tests__/UserStore.test.js @@ -0,0 +1,79 @@ +import fetch from 'jest-fetch-mock'; +import { UserStore } from '../UserStore'; + +global.fetch = fetch; + +function fakeUsers() { + return JSON.stringify([ + { name: 'Alice', username: 'alice', id: 1 }, + { name: 'Bob', username: 'bob', id: 2 }, + { name: 'Charlie', username: 'charlie', id: 3 }, + { name: 'Almost Alice', username: 'almostalice', id: 4 }, + ]); +} + +describe('UserStore', () => { + beforeEach(() => { + fetch.resetMocks(); + fetch.mockResponse(fakeUsers()); + }); + + test('initializes with an empty user list', () => { + const subject = new UserStore(); + expect(subject.users).toStrictEqual([]); + }); + + test('initializes unfetched', () => { + const subject = new UserStore(); + expect(subject.wasFetched).toBeFalsy(); + }); + + test('fetch from a given url', () => { + new UserStore().fetch('/path/to/the/users'); + expect(fetch).toHaveBeenCalledWith('/path/to/the/users'); + }); + + test('only fetches once', async () => { + const subject = new UserStore(); + await subject.fetch('/path/to/the/users'); + expect(subject.wasFetched).toBeTruthy(); + await subject.fetch('/path/to/the/users'); + await subject.fetch('/path/to/the/users'); + expect(fetch).toHaveBeenCalledTimes(1); + }); + + test('return a sub-set of users matching given IDs', async () => { + const subject = new UserStore(); + await subject.fetch('/path/to/the/users'); + expect(subject.matchingIds(['1', '4'])).toStrictEqual([ + { name: 'Alice', username: 'alice', id: 1 }, + { name: 'Almost Alice', username: 'almostalice', id: 4 }, + ]); + expect(subject.matchingIds(['2', '3'])).toStrictEqual([ + { name: 'Bob', username: 'bob', id: 2 }, + { name: 'Charlie', username: 'charlie', id: 3 }, + ]); + }); + + test('return a sub-set of users matching search term', async () => { + const subject = new UserStore(); + await subject.fetch('/path/to/the/users'); + expect(subject.search('alice')).toStrictEqual([ + { name: 'Alice', username: 'alice', id: 1 }, + { name: 'Almost Alice', username: 'almostalice', id: 4 }, + ]); + expect(subject.search('stal')).toStrictEqual([ + { name: 'Almost Alice', username: 'almostalice', id: 4 }, + ]); + expect(subject.search('david')).toStrictEqual([]); + }); + + test('search with an exception', async () => { + const subject = new UserStore(); + await subject.fetch('/path/to/the/users'); + expect(subject.search('a', { except: '3' })).toStrictEqual([ + { name: 'Alice', username: 'alice', id: 1 }, + { name: 'Almost Alice', username: 'almostalice', id: 4 }, + ]); + }); +}); diff --git a/app/javascript/shared/components/__tests__/UsernameInput.test.js b/app/javascript/shared/components/__tests__/UsernameInput.test.js new file mode 100644 index 000000000..8dc811667 --- /dev/null +++ b/app/javascript/shared/components/__tests__/UsernameInput.test.js @@ -0,0 +1,49 @@ +import { h } from 'preact'; +import { render } from '@testing-library/preact'; +import userEvent from '@testing-library/user-event'; + +import '@testing-library/jest-dom'; + +import { UsernameInput } from '../UsernameInput'; + +function fakeUsers() { + return [ + { name: 'Alice', username: 'alice', id: 1 }, + { name: 'Bob', username: 'bob', id: 2 }, + { name: 'Charlie', username: 'charlie', id: 3 }, + { name: 'Almost Alice', username: 'almostalice', id: 4 }, + ]; +} + +describe('', () => { + it('renders the default component', () => { + const { container } = render( + {}} + handleSelectionsChanged={() => {}} + />, + ); + expect(container.innerHTML).toMatchSnapshot(); + }); + + it('calls handleSelectionsChanged with user IDs', async () => { + const fakeHandler = jest.fn(); + + const { getByLabelText, queryByRole } = render( + , + ); + + const input = getByLabelText('Enter username'); + input.focus(); + await userEvent.type(input, 'Bob,'); + await userEvent.type(input, 'Charlie,'); + + expect(queryByRole('button', { name: 'Edit example' })).toBeNull(); + expect(fakeHandler).toHaveBeenCalledWith('2, 3'); + }); +}); diff --git a/app/javascript/shared/components/__tests__/__snapshots__/UsernameInput.test.js.snap b/app/javascript/shared/components/__tests__/__snapshots__/UsernameInput.test.js.snap new file mode 100644 index 000000000..fbace283b --- /dev/null +++ b/app/javascript/shared/components/__tests__/__snapshots__/UsernameInput.test.js.snap @@ -0,0 +1,3 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[` renders the default component 1`] = `"

        Selected items:

          "`; diff --git a/app/javascript/utilities/document_ready.js b/app/javascript/utilities/document_ready.js new file mode 100644 index 000000000..8986658f2 --- /dev/null +++ b/app/javascript/utilities/document_ready.js @@ -0,0 +1,7 @@ +Document.prototype.ready = new Promise((resolve) => { + if (document.readyState !== 'loading') { + return resolve(); + } + document.addEventListener('DOMContentLoaded', () => resolve()); + return null; +}); diff --git a/app/models/article.rb b/app/models/article.rb index c04743e93..e27bb6f4d 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -577,6 +577,10 @@ class Article < ApplicationRecord hotness_score: BlackBox.article_hotness_score(self)) end + def co_author_ids_list + co_author_ids.join(", ") + end + def co_author_ids_list=(list_of_co_author_ids) self.co_author_ids = list_of_co_author_ids.split(",").map(&:strip) end diff --git a/app/services/articles/attributes.rb b/app/services/articles/attributes.rb index 74ee89676..fef8a6a57 100644 --- a/app/services/articles/attributes.rb +++ b/app/services/articles/attributes.rb @@ -2,7 +2,7 @@ module Articles class Attributes ATTRIBUTES = %i[archived body_markdown canonical_url description edited_at main_image organization_id user_id published - title video_thumbnail_url published_at].freeze + title video_thumbnail_url published_at co_author_ids_list].freeze attr_reader :attributes, :article_user diff --git a/app/views/dashboards/_dashboard_article_row.html.erb b/app/views/dashboards/_dashboard_article_row.html.erb index 01704e306..da783056e 100644 --- a/app/views/dashboards/_dashboard_article_row.html.erb +++ b/app/views/dashboards/_dashboard_article_row.html.erb @@ -116,11 +116,26 @@ <% if organization && org_admin %> <%= form_for(article, html: { class: "mt-4 pt-4 border-0 border-t-1 border-color-base-10 border-solid" }) do |f| %> +
          <%= f.select :user_id, options_for_select(organization.users.pluck(:name, :id), article.user_id), {}, { class: "crayons-select" } %> <%= f.hidden_field :from_dashboard, value: 1 %>
          + +
          + +
          + <%= f.text_field :co_author_ids_list, + id: "article_#{article.id}_co_author_ids_list", + class: "article_org_co_author_ids_list", + data: { + fetch_users: organization_members_path(organization.username, format: "json") + } %> +
          + <%= f.hidden_field :from_dashboard, value: 1 %> +
          + <% end %> <% end %> diff --git a/app/views/dashboards/show.html.erb b/app/views/dashboards/show.html.erb index c50b60346..801ee7561 100644 --- a/app/views/dashboards/show.html.erb +++ b/app/views/dashboards/show.html.erb @@ -69,3 +69,4 @@ <%= javascript_packs_with_chunks_tag "archivedPostFilters", defer: true %> +<%= javascript_packs_with_chunks_tag "dashboards/convertCoauthorIdsToUsernameInputs", defer: true %> diff --git a/config/locales/views/dashboard/en.yml b/config/locales/views/dashboard/en.yml index 219430de9..cf34639bd 100644 --- a/config/locales/views/dashboard/en.yml +++ b/config/locales/views/dashboard/en.yml @@ -94,6 +94,7 @@ en: archive: Archive post unarchive: Unarchive post author: Author + co_authors: Co-authors author_is: 'AUTHOR: ' save: Save submit: Submit author change diff --git a/config/locales/views/dashboard/fr.yml b/config/locales/views/dashboard/fr.yml index 9781c500f..6eb92f1ff 100644 --- a/config/locales/views/dashboard/fr.yml +++ b/config/locales/views/dashboard/fr.yml @@ -94,6 +94,7 @@ fr: archive: Archive post unarchive: Unarchive post author: Author + co_authors: Co-authors author_is: 'AUTHOR: ' save: Save submit: Submit author change diff --git a/jest.config.js b/jest.config.js index 321f7d183..a24b07151 100644 --- a/jest.config.js +++ b/jest.config.js @@ -14,6 +14,7 @@ module.exports = { // This exclusion avoids running coverage on Barrel files, https://twitter.com/housecor/status/981558704708472832 '!app/javascript/**/index.js', '!app/javascript/packs/**/*.js', // avoids running coverage on webpacker pack files + 'app/javascript/packs/dashboards/*.js', // try opting-in for jest coverage? '!**/__tests__/**', '!**/__stories__/**', '!app/javascript/storybook-static/**/*.js',