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.

Sign up for GitHub

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

Jump to bottom

incoming email parsing feature for gmail and yahoo #2933

Merged
merged 12 commits into from
Jul 3, 2018
Merged

incoming email parsing feature for gmail and yahoo #2933

merged 12 commits into from
Jul 3, 2018

Conversation

namangupta01
Copy link
Member

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@ghost ghost assigned namangupta01 Jun 27, 2018
@ghost ghost added the in progress label Jun 27, 2018
@plotsbot
Copy link
Collaborator

plotsbot commented Jun 27, 2018 •
edited
Loading

2 Messages
📖 @namangupta01 Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@namangupta01 namangupta01 mentioned this pull request Jun 27, 2018
Closed
jywarren
jywarren reviewed Jun 27, 2018
View reviewed changes
db/migrate/20180627060150_add_column_reply_to_content_in_comment.rb Outdated
@@ -0,0 +1,5 @@
class AddColumnReplyToContentInComment < ActiveRecord::Migration[5.1]
def change
add_column :comments, :extra_content, :text
Copy link
Member

Choose a reason for hiding this comment

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

Could we just keep it in the regular comment body, but use a filter to remove it upon sending a notification email, and use JavaScript to hide it in the HTML views? Just wondering if we can solve this without a db change and the maintenance that may come with?

Copy link
Member Author

@namangupta01 namangupta01 Jun 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking, it will become complex if we try to fit both the content into one attribute..of course we can use filter but i was thinking that both the forms will have some maintenance but i am happy doing either ways....what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jywarren any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I guess I think we might use the render methods to offer a "full" or "trimmed" display for use in different places, and filter on display rather than filter on save. The filter could be like:

comment.filter_body(:gmail)
# vs
comment.body

maybe?

@jywarren
Copy link
Member

How's this coming along, Naman? Maybe we could get an early version published today?

@namangupta01
Copy link
Member Author

Yeah sure!

@namangupta01
Copy link
Member Author

namangupta01 commented Jun 29, 2018 via email

@namangupta01
Copy link
Member Author

Hey @jywarren, is this good now?

@jywarren
Copy link
Member

jywarren commented Jul 2, 2018

Hi @namangupta01 -- i think there are a few recommendations from CodeClimate that you could integrate -- not every one, but see if there are some you could resolve in code and the rest we can "accept".

Exciting, thanks!

@namangupta01
Copy link
Member Author

Yeah sure! I will fix them. Btw i was asking about the current implementation.

jywarren
jywarren reviewed Jul 2, 2018
View reviewed changes
app/models/comment.rb Outdated
@@ -13,6 +13,9 @@ class Comment < ApplicationRecord
self.table_name = 'comments'
self.primary_key = 'cid'

COMMENT_FILTER = "\n@@$$%% Trimmed Content @@$$%%\n"
Copy link
Member

Choose a reason for hiding this comment

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

Interesting and clever! What about putting it into an HTML comment as well - like <!-- $$%% TRIMMED CONTENT %%$$ --> so that it can be parsed out with an HTML filter if need be? I dunno, just seems potentially extra helpful in the future...

Copy link
Member Author

Choose a reason for hiding this comment

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

This way would be great. Putting this in html comment didn't come to my mind. 👍 🎉

@jywarren
Copy link
Member

jywarren commented Jul 2, 2018

Awesome, this looks good! Did you want to perhaps include sample reply emails from diff. services in tests, and confirm they get stripped properly? This will also have the added benefit that if email vendors change their formats, we can compare to the test examples to update the solution.

Great work! Staff were VERY excited to hear about this feature -- open mouthed! 😮

@namangupta01
Copy link
Member Author

Yes, i was already planning to do these and some more testing if we can find a way to test them in another pr.

@jywarren
Copy link
Member

jywarren commented Jul 2, 2018

OK - do you want to merge this first then add tests in another PR? I think it can be very good to have tests be in the same PR so there's a relationship between the feature and the test that confirms it... what do you think?

@jywarren
Copy link
Member

jywarren commented Jul 2, 2018

Even just adding some basic tests of this -- they can be expanded later!

@namangupta01
Copy link
Member Author

namangupta01 commented Jul 2, 2018 via email

@namangupta01
Copy link
Member Author

Just for demo:

ezgif com-video-to-gif 8

@jywarren
Copy link
Member

jywarren commented Jul 2, 2018

oooooooooohhhhhhhh 👍 👍 👍 fantastic!!

@jywarren
Copy link
Member

jywarren commented Jul 3, 2018

I think this may need to be rebased following @Souravirus upgrade to Rails 5.2 -- @Souravirus what do you think?

@namangupta01
Copy link
Member Author

@jywarren All test are failing on local. I am using bundle exec ruby -I test test/unit/comment_test.rb but all tests are failing any ideas?

@namangupta01
Copy link
Member Author

Got it...never mind

@namangupta01
Copy link
Member Author

@jywarren I have added the test... can you please have a look so that i can finalize it?

@namangupta01
Copy link
Member Author

@jywarren Is it good now?

@namangupta01
Copy link
Member Author

Also added readme for google and yahoo parsing.

@jywarren
Copy link
Member

jywarren commented Jul 3, 2018

Awesome. Fantastic work!!!

@jywarren jywarren merged commit 8237d80 into master Jul 3, 2018
@ghost ghost removed the in progress label Jul 3, 2018
@jywarren
Copy link
Member

jywarren commented Jul 3, 2018

And congrats on this -- very well implemented! Let's keep an eye out as it goes live since it's rather complex, and we can patch anything that comes up. Thanks!

@namangupta01
Copy link
Member Author

Yeah sure and we will also expand this feature for all email services.

@namangupta01 namangupta01 mentioned this pull request Jul 9, 2018
Closed
namangupta01 added a commit to namangupta01/plots2 that referenced this pull request Jul 9, 2018
@namangupta01
Added remaining unit tests for publiclab#2933
00d0e47
namangupta01 added a commit to namangupta01/plots2 that referenced this pull request Jul 12, 2018
@namangupta01
Added remaining unit tests for publiclab#2933
06c10bd
jywarren pushed a commit that referenced this pull request Jul 12, 2018
@namangupta01 @jywarren
Added remaining unit tests for #2933 (#3020)
3c79545 
* Added remaining unit tests for #2933

* Minor change

* Indentation changed

* Added tests and moved emails to fixtures

* Added tests and moved emails to fixtures
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
@namangupta01 @jywarren
incoming email parsing feature for gmail and yahoo (publiclab#2933)
6843e2f 
* Added reply to content field in comment table

* Added incoming email parsing feature for gmail and yahooo

* Added incoming email parsing feature for gmail and yahooo

* Minor changes

* Removed column

* Added filter in comment

* Added mail filter in mails

* Minor changes

* Fixes codeclimate issues

* Modified COMMENT_FILTER

* Added test for parsing gmail and yahoo

* Added detail readme for yahoo and gmail parsing
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
@namangupta01 @jywarren
Added remaining unit tests for publiclab#2933 (publiclab#3020)
c31b7f6 
* Added remaining unit tests for publiclab#2933

* Minor change

* Indentation changed

* Added tests and moved emails to fixtures

* Added tests and moved emails to fixtures
@emilyashley emilyashley deleted the parse_mail branch January 15, 2020 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jywarren jywarren jywarren left review comments

Assignees

@namangupta01 namangupta01

Labels
summer-of-code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@namangupta01 @plotsbot @jywarren