Skip to content
This repository was archived by the owner on May 11, 2021. It is now read-only.

Issue 204 #205

Merged
merged 4 commits into from
Nov 13, 2019
Merged

Issue 204 #205

merged 4 commits into from
Nov 13, 2019

Conversation

jdudley1123
Copy link
Member

Issue

Data in Trix rich-text fields on debriefs was not getting submitted, causing validation errors that prevented debriefs from being submitted (#204). The root cause of this is an open issue on trix-rails.

Fix

Addressing this by manually specifying the input attribute of the <trix-editor> tag should have fixed this, but the version of trix-rails we were using was out of date. However, upgrading to the latest version (2.2.0, Jul 2018) didn't work either because the fix (Oct 2018) is in the master branch, but not in a release. So I changed our Gemfile to use the master branch. This got manually specifying the input working and fixed the issue.

Tests

The issue hadn't been flagged by any of our tests, because our tests do not use a debrief with ratings, and therefore the offending rich-text field is never shown.

I updated our tests to use a :committed_iteration factory in debrief test setup rather than a plain iteration. This broke all the other debrief tests which don't fill out the new field, so the submit_debrief method had to be updated for the new default behaviour and split into fill_debrief and submit_debrief to allow us to make changes to the form contents in specific tests.

Because the new field uses a <trix-editor> tag, Capybara can't interact with it in the usual way. Since there are two <trix-editor> tags on the page (one for the rating comments, one for the debrief notes), and they don't have IDs, I used first('trix-editor') to get the right one.

Additionally, there's also a new radio field which means Capybara can't distinguish between the outcome rating radio selection and the milestone completion radio selection. This necessitated switching from choose 'Yes' to choose 'debrief_milestone_completed_true', specifying by field ID.

This kind of issue should now be picked up by multiple tests as debrief system tests now more closely follow real user behaviour (ie. debriefing iterations that have outcomes).

Conclusion

Make sure your tests reflect real user behaviour and most likely scenarios first and foremost. Having the debrief system test use an iteration without outcomes made writing those initial tests quicker, but allowed us to ship a broken product and cost us time in the long run.

Time to fix error: ~15 mins
Time to get tests working: ~1 hour

Copy link
Contributor

@suninthesky suninthesky left a comment

Choose a reason for hiding this comment

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

Great fix and documentation!

@jdudley1123 jdudley1123 merged commit 6bee1b6 into master Nov 13, 2019
@jdudley1123 jdudley1123 deleted the issue-204 branch November 13, 2019 16:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants