From aa53a1c71f630798610dbd274006b90be22d3998 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Wed, 7 Sep 2022 17:51:46 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20error=20when=20deleting?= =?UTF-8?q?=20tag=20and=20missing=20slugs=20on=20tags=20list?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no issue - fixes error that left the confirmation modal in place when deleting a tag by ensuring we return `true` in the task used by the confirm button, if we return the transition object it trips the "failed" state because the `/tags` route aborts and refreshes when transitioning to it - fixes missing attached posts count in the tag delete confirmation modal by using the correct `tag.count.posts` attribute in the conditional - fixes missing slugs in the tags list by using the properties on `@tag` rather than expecting a separate `@slug` argument - replaced the skipped tags acceptance tests with an updated tests that match the recent redesign --- .../app/components/gh-tag-settings-form.hbs | 2 + .../app/components/gh-tags-list-item.hbs | 8 +- .../admin/app/components/modal-delete-tag.hbs | 8 +- .../admin/app/components/modal-delete-tag.js | 3 + ghost/admin/app/controllers/tag.js | 3 +- ghost/admin/app/templates/tag.hbs | 2 +- ghost/admin/app/templates/tags.hbs | 6 +- .../tests/acceptance/settings/tags-test.js | 285 ++++-------------- 8 files changed, 84 insertions(+), 233 deletions(-) diff --git a/ghost/admin/app/components/gh-tag-settings-form.hbs b/ghost/admin/app/components/gh-tag-settings-form.hbs index 3ad4ae2059..367e926611 100644 --- a/ghost/admin/app/components/gh-tag-settings-form.hbs +++ b/ghost/admin/app/components/gh-tag-settings-form.hbs @@ -12,6 +12,7 @@ @value={{this.scratchTag.name}} @tabindex="1" @focus-out={{action "setProperty" "name" this.scratchTag.name}} + data-test-input="tag-name" /> @@ -63,6 +64,7 @@ @name="slug" @tabindex="2" @focus-out={{action "setProperty" "slug" this.scratchTag.slug}} + data-test-input="tag-slug" /> diff --git a/ghost/admin/app/components/gh-tags-list-item.hbs b/ghost/admin/app/components/gh-tags-list-item.hbs index c858ac9540..5baddfe10a 100644 --- a/ghost/admin/app/components/gh-tags-list-item.hbs +++ b/ghost/admin/app/components/gh-tags-list-item.hbs @@ -1,17 +1,17 @@
  • -

    +

    {{@tag.name}}

    {{#if @tag.description}} -

    +

    {{@tag.description}}

    {{/if}}     - - {{@slug}} + + {{@tag.slug}} {{#if @tag.count.posts}} diff --git a/ghost/admin/app/components/modal-delete-tag.hbs b/ghost/admin/app/components/modal-delete-tag.hbs index 968c1b5f59..808d8972f6 100644 --- a/ghost/admin/app/components/modal-delete-tag.hbs +++ b/ghost/admin/app/components/modal-delete-tag.hbs @@ -4,13 +4,13 @@ {{svg-jar "close"}}
    - {{#if this.tag.post_count}} - This tag is attached to {{this.tag.count.posts}} {{this.postInflection}}. + {{#if this.tag.count.posts}} + This tag is attached to {{this.tag.count.posts}} {{this.postInflection}}. {{/if}} You're about to delete "{{this.tag.name}}". This is permanent! We warned you, k?
    - - + +
    diff --git a/ghost/admin/app/components/modal-delete-tag.js b/ghost/admin/app/components/modal-delete-tag.js index e1d4a459a0..9118fc616d 100644 --- a/ghost/admin/app/components/modal-delete-tag.js +++ b/ghost/admin/app/components/modal-delete-tag.js @@ -4,6 +4,9 @@ import {computed} from '@ember/object'; import {task} from 'ember-concurrency'; export default ModalComponent.extend({ + attributeBindings: ['dataTestModal:data-test-modal'], + dataTestModal: 'confirm-delete-tag', + // Allowed actions confirm: () => {}, diff --git a/ghost/admin/app/controllers/tag.js b/ghost/admin/app/controllers/tag.js index 56f73f120a..c75825c12b 100644 --- a/ghost/admin/app/controllers/tag.js +++ b/ghost/admin/app/controllers/tag.js @@ -45,7 +45,8 @@ export default class TagController extends Controller { deleteTag() { return this.tag.destroyRecord().then(() => { this.set('showDeleteTagModal', false); - return this.transitionToRoute('tags'); + this.router.transitionTo('tags'); + return true; }, (error) => { return this.notifications.showAPIError(error, {key: 'tag.delete'}); }); diff --git a/ghost/admin/app/templates/tag.hbs b/ghost/admin/app/templates/tag.hbs index 0e6538a824..04cc2e25d2 100644 --- a/ghost/admin/app/templates/tag.hbs +++ b/ghost/admin/app/templates/tag.hbs @@ -19,7 +19,7 @@ {{#unless this.tag.isNew}}
    -
    diff --git a/ghost/admin/app/templates/tags.hbs b/ghost/admin/app/templates/tags.hbs index ebf4d2e1ed..f144dc3bf7 100644 --- a/ghost/admin/app/templates/tags.hbs +++ b/ghost/admin/app/templates/tags.hbs @@ -3,8 +3,8 @@

    Tags

    - - + +
    New tag
    @@ -20,7 +20,7 @@
    • - + {{else}}
  • diff --git a/ghost/admin/tests/acceptance/settings/tags-test.js b/ghost/admin/tests/acceptance/settings/tags-test.js index a2d0f176d7..6c1a86cc1e 100644 --- a/ghost/admin/tests/acceptance/settings/tags-test.js +++ b/ghost/admin/tests/acceptance/settings/tags-test.js @@ -10,34 +10,7 @@ import {setupMirage} from 'ember-cli-mirage/test-support'; import {timeout} from 'ember-concurrency'; import {visit} from '../../helpers/visit'; -// Grabbed from keymaster's testing code because Ember's `keyEvent` helper -// is for some reason not triggering the events in a way that keymaster detects: -// https://github.com/madrobby/keymaster/blob/master/test/keymaster.html#L31 -const modifierMap = { - 16: 'shiftKey', - 18: 'altKey', - 17: 'ctrlKey', - 91: 'metaKey' -}; -let keydown = function (code, modifiers, el) { - let event = document.createEvent('Event'); - event.initEvent('keydown', true, true); - event.keyCode = code; - if (modifiers && modifiers.length > 0) { - for (let i in modifiers) { - event[modifierMap[modifiers[i]]] = true; - } - } - (el || document).dispatchEvent(event); -}; -let keyup = function (code, el) { - let event = document.createEvent('Event'); - event.initEvent('keyup', true, true); - event.keyCode = code; - (el || document).dispatchEvent(event); -}; - -describe.skip('Acceptance: Tags', function () { +describe('Acceptance: Tags', function () { let hooks = setupApplicationTest(); setupMirage(hooks); @@ -88,17 +61,18 @@ describe.skip('Acceptance: Tags', function () { windowProxy.replaceState = originalReplaceState; }); - it('it renders, can be navigated, can edit, create & delete tags', async function () { - let tag1 = this.server.create('tag'); - let tag2 = this.server.create('tag'); + it('lists public and internal tags separately', async function () { + this.server.create('tag', {name: 'B - Third', slug: 'third'}); + this.server.create('tag', {name: 'Z - Last', slug: 'last'}); + this.server.create('tag', {name: '!A - Second', slug: 'second'}); + this.server.create('tag', {name: 'A - First', slug: 'first'}); + this.server.create('tag', {name: '#one', slug: 'hash-one', visibility: 'internal'}); + this.server.create('tag', {name: '#two', slug: 'hash-two', visibility: 'internal'}); - await visit('/tags'); + await visit('tags'); - // it redirects to first tag - // expect(currentURL(), 'currentURL').to.equal(`/tags/${tag1.slug}`); - - // it doesn't redirect to first tag - expect(currentURL(), 'currentURL').to.equal('/tags'); + // it loads tags list + expect(currentURL(), 'currentURL').to.equal('tags'); // it has correct page title expect(document.title, 'page title').to.equal('Tags - Test Blog'); @@ -107,197 +81,84 @@ describe.skip('Acceptance: Tags', function () { expect(find('[data-test-nav="tags"]'), 'highlights nav menu item') .to.have.class('active'); - // it lists all tags - expect(findAll('.tags-list .gh-tags-list-item').length, 'tag list count') - .to.equal(2); - let tag = find('.tags-list .gh-tags-list-item'); - expect(tag.querySelector('.gh-tag-list-name').textContent, 'tag list item title') - .to.equal(tag1.name); + // it defaults to public tags + expect(find('[data-test-tags-nav="public"]')).to.have.attr('data-test-active'); + expect(find('[data-test-tags-nav="internal"]')).to.not.have.attr('data-test-active'); - // it highlights selected tag - // expect(find(`a[href="/ghost/tags/${tag1.slug}"]`), 'highlights selected tag') - // .to.have.class('active'); + // it lists all public tags + expect(findAll('[data-test-tag]'), 'public tag list count') + .to.have.length(4); - await visit(`/tags/${tag1.slug}`); + // tags are in correct order + let tags = findAll('[data-test-tag]'); - // second wait is needed for the tag details to settle + expect(tags[0].querySelector('[data-test-tag-name]')).to.have.trimmed.text('A - First'); + expect(tags[1].querySelector('[data-test-tag-name]')).to.have.trimmed.text('!A - Second'); + expect(tags[2].querySelector('[data-test-tag-name]')).to.have.trimmed.text('B - Third'); + expect(tags[3].querySelector('[data-test-tag-name]')).to.have.trimmed.text('Z - Last'); - // it shows selected tag form - // expect(find('.tag-settings-pane h4').textContent, 'settings pane title') - // .to.equal('Tag settings'); - expect(find('.gh-tag-basic-settings-form input[name="name"]').value, 'loads correct tag into form') - .to.equal(tag1.name); + // can switch to internal tags + await click('[data-test-tags-nav="internal"]'); - // click the second tag in the list - // let tagEditButtons = findAll('.tag-edit-button'); - // await click(tagEditButtons[tagEditButtons.length - 1]); - - // it navigates to selected tag - // expect(currentURL(), 'url after clicking tag').to.equal(`/tags/${tag2.slug}`); - - // it highlights selected tag - // expect(find(`a[href="/ghost/tags/${tag2.slug}"]`), 'highlights selected tag') - // .to.have.class('active'); - - // it shows selected tag form - // expect(find('.tag-settings-pane input[name="name"]').value, 'loads correct tag into form') - // .to.equal(tag2.name); - - // simulate up arrow press - run(() => { - keydown(38); - keyup(38); - }); - - await settled(); - - // it navigates to previous tag - expect(currentURL(), 'url after keyboard up arrow').to.equal(`/tags/${tag1.slug}`); - - // it highlights selected tag - // expect(find(`a[href="/ghost/tags/${tag1.slug}"]`), 'selects previous tag') - // .to.have.class('active'); - - // simulate down arrow press - run(() => { - keydown(40); - keyup(40); - }); - - await settled(); - - // it navigates to previous tag - expect(currentURL(), 'url after keyboard down arrow').to.equal(`/tags/${tag2.slug}`); - - // it highlights selected tag - // expect(find(`a[href="/ghost/tags/${tag2.slug}"]`), 'selects next tag') - // .to.have.class('active'); - - // trigger save - await fillIn('.tag-settings-pane input[name="name"]', 'New Name'); - await blur('.tag-settings-pane input[name="name"]'); - - // extra timeout needed for Travis - sometimes it doesn't update - // quick enough and an extra wait() call doesn't help - await timeout(100); - - // check we update with the data returned from the server - let tags = findAll('.settings-tags .settings-tag'); - tag = tags[0]; - expect(tag.querySelector('.tag-title').textContent, 'tag list updates on save') - .to.equal('New Name'); - expect(find('.tag-settings-pane input[name="name"]').value, 'settings form updates on save') - .to.equal('New Name'); - - // start new tag - await click('.view-actions .gh-btn-green'); - - // it navigates to the new tag route - expect(currentURL(), 'new tag URL').to.equal('/tags/new'); - - // it displays the new tag form - expect(find('.tag-settings-pane h4').textContent, 'settings pane title') - .to.equal('New tag'); - - // all fields start blank - findAll('.tag-settings-pane input, .tag-settings-pane textarea').forEach(function (elem) { - expect(elem.value, `input field for ${elem.getAttribute('name')}`) - .to.be.empty; - }); - - // save new tag - await fillIn('.tag-settings-pane input[name="name"]', 'New tag'); - await blur('.tag-settings-pane input[name="name"]'); - - // extra timeout needed for FF on Linux - sometimes it doesn't update - // quick enough, especially on Travis, and an extra wait() call - // doesn't help - await timeout(100); - - // it redirects to the new tag's URL - expect(currentURL(), 'URL after tag creation').to.equal('/tags/new-tag'); - - // it adds the tag to the list and selects - tags = findAll('.settings-tags .settings-tag'); - tag = tags[1]; // second tag in list due to alphabetical ordering - expect(tags.length, 'tag list count after creation') - .to.equal(3); - - // new tag will be second in the list due to alphabetical sorting - expect(findAll('.settings-tags .settings-tag')[1].querySelector('.tag-title').textContent.trim(), 'new tag list item title'); - expect(tag.querySelector('.tag-title').textContent, 'new tag list item title') - .to.equal('New tag'); - expect(find('a[href="/ghost/tags/new-tag"]'), 'highlights new tag') - .to.have.class('active'); - - // delete tag - await click('.settings-menu-delete-button'); - await click('.fullscreen-modal .gh-btn-red'); - - // it redirects to the first tag - expect(currentURL(), 'URL after tag deletion').to.equal(`/tags/${tag1.slug}`); - - // it removes the tag from the list - expect(findAll('.settings-tags .settings-tag').length, 'tag list count after deletion') - .to.equal(2); + expect(findAll('[data-test-tag]'), 'internal tag list count').to.have.length(2); }); - // TODO: Unskip and fix - // skipped because it was failing most of the time on Travis - // see https://github.com/TryGhost/Ghost/issues/8805 - it.skip('loads tag via slug when accessed directly', async function () { - this.server.createList('tag', 2); + it('can edit tags', async function () { + const tag = this.server.create('tag', {name: 'To be edited', slug: 'to-be-edited'}); - await visit('/tags/tag-1'); + await visit('tags'); + await click(`[data-test-tag="${tag.id}"] [data-test-tag-name]`); - expect(currentURL(), 'URL after direct load').to.equal('/tags/tag-1'); + expect(currentURL()).to.equal('/tags/to-be-edited'); - // it loads all other tags - expect(findAll('.settings-tags .settings-tag').length, 'tag list count after direct load') - .to.equal(2); + expect(find('[data-test-input="tag-name"]')).to.have.value('To be edited'); + expect(find('[data-test-input="tag-slug"]')).to.have.value('to-be-edited'); - // selects tag in list - expect(find('a[href="/ghost/tags/tag-1"]').classList.contains('active'), 'highlights requested tag') - .to.be.true; + await fillIn('[data-test-input="tag-name"]', 'New tag name'); + await fillIn('[data-test-input="tag-slug"]', 'new-tag-slug'); + await click('[data-test-button="save"]'); - // shows requested tag in settings pane - expect(find('.tag-settings-pane input[name="name"]').value, 'loads correct tag into form') - .to.equal('Tag 1'); + const savedTag = this.server.db.tags.find(tag.id); + expect(savedTag.name, 'saved tag name').to.equal('New tag name'); + expect(savedTag.slug, 'saved tag slug').to.equal('new-tag-slug'); + + await click('[data-test-link="tags-back"]'); + + const tagListItem = find('[data-test-tag]'); + expect(tagListItem.querySelector('[data-test-tag-name]')).to.have.trimmed.text('New tag name'); + expect(tagListItem.querySelector('[data-test-tag-slug]')).to.have.trimmed.text('new-tag-slug'); }); - it('shows the internal tag label', async function () { - this.server.create('tag', {name: '#internal-tag', slug: 'hash-internal-tag', visibility: 'internal'}); + it('can delete tags', async function () { + const tag = this.server.create('tag', {name: 'To be edited', slug: 'to-be-edited'}); + this.server.create('post', {tags: [tag]}); - await visit('tags/'); + await visit('tags'); + await click(`[data-test-tag="${tag.id}"] [data-test-tag-name]`); - expect(currentURL()).to.equal('/tags/hash-internal-tag'); + await click('[data-test-button="delete-tag"]'); - expect(findAll('.settings-tags .settings-tag').length, 'tag list count') - .to.equal(1); + const tagModal = '[data-test-modal="confirm-delete-tag"]'; - let tag = find('.settings-tags .settings-tag'); + expect(find(tagModal)).to.exist; + expect(find(`${tagModal} [data-test-text="posts-count"]`)) + .to.have.trimmed.text('1 post'); - expect(tag.querySelectorAll('.label.label-blue').length, 'internal tag label') - .to.equal(1); + await click(`${tagModal} [data-test-button="confirm"]`); - expect(tag.querySelector('.label.label-blue').textContent.trim(), 'internal tag label text') - .to.equal('internal'); + expect(find(tagModal)).to.not.exist; + expect(currentURL()).to.equal('/tags'); + expect(findAll('[data-test-tag]')).to.have.length(0); }); - it('updates the URL when slug changes', async function () { - this.server.createList('tag', 2); + it('can load tag via slug in url', async function () { + const tag = this.server.create('tag', {name: 'To be edited', slug: 'to-be-edited'}); - await visit('/tags/tag-1'); + await visit('tags/to-be-edited'); + expect(currentURL()).to.equal('tags/to-be-edited'); - expect(currentURL(), 'URL after direct load').to.equal('/tags/tag-1'); - - // update the slug - await fillIn('.tag-settings-pane input[name="slug"]', 'test'); - await blur('.tag-settings-pane input[name="slug"]'); - - // tests don't have a location.hash so we can only check that the - // slug portion is updated correctly - expect(newLocation, 'URL after slug change').to.equal('test'); + expect(find('[data-test-input="tag-name"]')).to.have.value('To be edited'); + expect(find('[data-test-input="tag-slug"]')).to.have.value('to-be-edited'); }); it('redirects to 404 when tag does not exist', async function () { @@ -310,21 +171,5 @@ describe.skip('Acceptance: Tags', function () { expect(currentRouteName()).to.equal('error404'); expect(currentURL()).to.equal('/tags/unknown'); }); - - it('sorts tags correctly', async function () { - this.server.create('tag', {name: 'B - Third', slug: 'third'}); - this.server.create('tag', {name: 'Z - Last', slug: 'last'}); - this.server.create('tag', {name: '#A - Second', slug: 'second'}); - this.server.create('tag', {name: 'A - First', slug: 'first'}); - - await visit('tags'); - - let tags = findAll('[data-test-tag]'); - - expect(tags[0].querySelector('[data-test-name]').textContent.trim()).to.equal('A - First'); - expect(tags[1].querySelector('[data-test-name]').textContent.trim()).to.equal('#A - Second'); - expect(tags[2].querySelector('[data-test-name]').textContent.trim()).to.equal('B - Third'); - expect(tags[3].querySelector('[data-test-name]').textContent.trim()).to.equal('Z - Last'); - }); }); });