New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken link to remove tags from entries #3536

Merged
merged 3 commits into from Jan 3, 2018

Conversation

Projects
None yet
4 participants
@Kdecherf
Contributor

Kdecherf commented Dec 27, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? -
Documentation no
Translation no
CHANGELOG.md no
Fixed tickets #3534
License MIT

Fixes #3534

@Kdecherf Kdecherf added this to the 2.3.2 milestone Dec 27, 2017

@Kdecherf Kdecherf requested review from j0k3r, nicosomb, Simounet and tcitworld Dec 27, 2017

@Simounet

I would prefer a more describing variable than eid like entryId but it looks good to me. Sorry about that.

Fix broken link to remove tags from entries
Fixes #3534

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

@Kdecherf Kdecherf force-pushed the tag-link-3534 branch from 83d640c to 8e15ece Dec 27, 2017

@Kdecherf

This comment has been minimized.

Contributor

Kdecherf commented Dec 27, 2017

@Simounet updated, thanks

@nicosomb

This comment has been minimized.

Member

nicosomb commented Dec 27, 2017

Do you think that it's easy to add a test to avoid regressions again here?

Add test to prevent regression for #3534
Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
@Kdecherf

This comment has been minimized.

Contributor

Kdecherf commented Dec 30, 2017

Done, there's a test now for this issue

// the deletion link of the first tag
$link = $crawler->filter('body div#article div.tools ul.tags li.chip a')->extract('href')[1];
$this->assertSame(sprintf('/remove-tag/%s/%s', $entry->getId(), $tag->getId()), $link);

This comment has been minimized.

@j0k3r

j0k3r Dec 31, 2017

Member

Why don't you follow the link to ensure it returns a 301?

This comment has been minimized.

@Kdecherf

Kdecherf Dec 31, 2017

Contributor

Do you want a complete test in the same PR or can we delay it to another PR?

This comment has been minimized.

@j0k3r

j0k3r Jan 1, 2018

Member

As you want 🎅

php-cs
Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

@nicosomb nicosomb merged commit 410216f into master Jan 3, 2018

5 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
zappr/pr/specification PR has passed specification checks
zappr/pr/tasks PR has no open tasks.

@nicosomb nicosomb deleted the tag-link-3534 branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment