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

Hide earlier emails in a fact check received email chain #308

Merged
merged 6 commits into from Nov 26, 2014

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Nov 24, 2014

https://www.agileplannerapp.com/boards/173808/cards/8129

Avoid reproducing the original fact check email, and every other email in the reply chain, within the edition history and notes view. Instead show the most recent reply and hide all earlier emails behind a toggle, so that no information is lost.

Email detection is based on common patterns used by Outlook, OSX Mail, iPhone and BlackBerry. More can be added fairly easily, but this seems to have captured the majority of formats within existing notes.

The style of fact check emails has also been tweaked so that they appear different to regular notes, and can either be scrolled to or past with ease.

Email

screen shot 2014-11-24 at 12 52 13

Expanded email

screen shot 2014-11-24 at 12 52 51

fofr added 4 commits Nov 21, 2014
Pull out fact check emails so they’re displayed differently. This
should make it easier to scroll past them when they’re very long, make
them easier to find in the list and reduce the space they take up by
reducing their font size.
* Only show the latest response in the history and notes feed to
prevent pages of scrolling
* Keep the full original email available behind a toggle, so it can be
viewed if necessary and so that no information is unavailable
* Works for common reply formats from outlook, OSX mail, iPhone and
Blackberry
* Ignores everything but the first instance of an original email
Refer to earlier messages rather than original message. There may be
more messages in the reply chain, some of which might have worthwhile
content.
@@ -24,6 +24,8 @@ class EditionHistoryTest < JavascriptIntegrationTest
@guide.new_action(@author, Action::NOTE, {:comment => "link http://www.some-link.com"})

assert_equal ["fourth", "fifth", "sixth", "link http://www.some-link.com"], @guide.actions.map(&:comment)

@guide.new_action(@author, Action::RECEIVE_FACT_CHECK, {:comment => "email reply\n-----Original Message-----\noriginal email request"})

This comment has been minimized.

@vinayvinay

vinayvinay Nov 26, 2014
Contributor

should this new action be setup within the newly added test, the other tests don't seem to use this action?

unformatted_email = split_email_at_reply(text)
assert_equal unformatted_email.length, 3
assert_match /reply/, unformatted_email.first
assert_match /original/, unformatted_email.last

This comment has been minimized.

@vinayvinay

vinayvinay Nov 26, 2014
Contributor

these could be made more succinct by assert_equal ["reply\n", "-----Original Message-----", "\noriginal"], split_email_at_reply(text). i understand that the separator is not an important bit to test, but whether \n gets picked-up is necessary, which may get lost while doing an assert_match.

This comment has been minimized.

@vinayvinay

vinayvinay Nov 26, 2014
Contributor

sorry, ignore that please. i only worked my way through the first iteration ;)


# Based on common reply patterns
# http://stackoverflow.com/questions/1372694/strip-signatures-and-replies-from-emails
# https://github.com/fofr/email_reply_parser/blob/master/lib/email_reply_parser.rb

This comment has been minimized.

@vinayvinay

vinayvinay Nov 26, 2014
Contributor

can we reference https://github.com/github/email_reply_parser... here instead, given that the fork has not diverged from github:master?

This comment has been minimized.

@fofr

fofr Nov 26, 2014
Author Contributor

Good spot, my mistake. Fixed.

@vinayvinay vinayvinay self-assigned this Nov 26, 2014
@vinayvinay
Copy link
Contributor

@vinayvinay vinayvinay commented Nov 26, 2014

@fofr: apart from the comment about test setup, this looks good to me (in the browser, i meant :).

vinayvinay added a commit that referenced this pull request Nov 26, 2014
Hide earlier emails in a fact check received email chain
@vinayvinay vinayvinay merged commit 02ec23f into master Nov 26, 2014
1 check passed
1 check passed
@govuk-ci
default "Build #472 succeeded on Jenkins"
Details
@vinayvinay vinayvinay deleted the style-fact-check-emails branch Nov 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants