Skip to content
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 deleting sublinks #733

Merged
merged 1 commit into from
May 10, 2019
Merged

Fix deleting sublinks #733

merged 1 commit into from
May 10, 2019

Conversation

Reettaphant
Copy link
Contributor

What's changed?

Detail the main feature of this PR and optionally a link to the relevant issue / card

I broke deleting sublinks, I fixed deleting sublinks. We can't select a group if we are deleting a sublinks and we need to handle this case correctly.

Implementation notes

Include any specific areas you want to highlight for review that you feel might be worthy of discussion (i.e. any non-obvious decisions you've made)

Checklist

General

  • 🤖 Relevant tests added
  • ✅ CI checks / tests run locally
  • 🔍 Checked on CODE

Client

  • 🚫 No obvious console errors on the client (i.e. React dev mode errors)
  • 🎛️ No regressions with existing user interactions (i.e. all existing buttons, inputs etc. work)
  • 📷 Screenshots / GIFs of relevant UI changes included

@jonathonherbert
Copy link
Contributor

Oh no, good to see a fix! Is it possible to test this behaviour via either unit/integration? We've introduced a few regressions recently and it'd be good to catch them next time.

@Reettaphant
Copy link
Contributor Author

I think for finding these kinds fo bugs integration tests testing removing, adding and moving articles from different positions would be very useful for catching these kinds of issues and we should do them before news desk rollout so that we stop breaking move beahviour. I think it's out of scope for this pr though.

Copy link
Contributor

@RichieAHB RichieAHB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should get this out, but I do think this probably needs a test given the amount of logic here.

@Reettaphant Reettaphant merged commit ac23593 into master May 10, 2019
@Reettaphant Reettaphant deleted the delete-article-fragments branch May 10, 2019 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants