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

Email digest follow-up changes list #4732

Closed
9 tasks done
jywarren opened this issue Jan 30, 2019 · 55 comments · Fixed by #4780, #4875, #5638 or #5907
Closed
9 tasks done

Email digest follow-up changes list #4732

jywarren opened this issue Jan 30, 2019 · 55 comments · Fixed by #4780, #4875, #5638 or #5907
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration help wanted requires help by anyone willing to contribute HTML planning Planning issues!

Comments

@jywarren
Copy link
Member

jywarren commented Jan 30, 2019 •
edited
Loading

Our daily email digest is now working! Now we have a few follow ups to do.

Overall this is very exciting!
@jywarren
Copy link
Member Author

@ViditChitkara check it out!

screenshot_20190129-221111

screenshot_20190129-221118

@jywarren jywarren added help wanted requires help by anyone willing to contribute break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration HTML labels Jan 30, 2019
@jywarren
Copy link
Member Author

Also linking with #2378 and with the browser based view of the digest at https://publiclab.org/subscriptions/digest

@grvsachdeva grvsachdeva added the planning Planning issues! label Jan 30, 2019
@arungoel123456
Copy link
Contributor

@jywarren I wanna work on this issue

@jywarren
Copy link
Member Author

jywarren commented Feb 1, 2019 via email

@ViditChitkara
Copy link
Member

Wow, feels really good to see this working @jywarren. @arungoel123456 in my opinion you may start with dates and label of mail. This is what I feel would be a good starting point. It's totally up to you if you want to start with another. Ping me if you need any help with this.

@IshaGupta18
Copy link
Collaborator

@ViditChitkara @jywarren could I work on some other parts of this issue?

@grvsachdeva
Copy link
Member

Hi @IshaGupta18, feel free to proceed. But, before working on a part please drop a message here to let others know, to avoid any conflict.

Also, @arungoel123456 and @IshaGupta18 you can discuss and divide parts here. Thanks!

@IshaGupta18
Copy link
Collaborator

Yes absoluely @gauravano. Thanks a lot! I actually found the places where the 'daily digest is labeled weekly' and 'dates on posts don't seem right' are mentioned. Maybe I can start with them, if @arungoel123456 hasn't already done that? Please let me know in either case.

@IshaGupta18
Copy link
Collaborator

I think that 'are we sure the same posts aren't being sent out multiple times?' would be a relatively harder one, because we'll have to keep a record of what content has already been sent in the previous day's mail and a mail should be sent on that day if and only is the content is slightly different from the one sent the day before. I have received I think 2-3 redundant mails for a tag.

@arungoel123456
Copy link
Contributor

@IshaGupta18 I would like to work on the date and label issue .... Sorry for the late reply

@arungoel123456
Copy link
Contributor

@ViditChitkara I will sure contact u if I need some help . Thanks for advising

@jywarren
Copy link
Member Author

jywarren commented Feb 4, 2019 via email

@IshaGupta18
Copy link
Collaborator

@arungoel123456 kindly proceed then! @jywarren I think that could be great idea!

@IshaGupta18
Copy link
Collaborator

I think for the width related problems, we can do 2 things:

  1. Make the existing code responsive
  2. Make a new email format using tables and make that responsive

What's your take on this @ViditChitkara @jywarren ?

@jywarren
Copy link
Member Author

jywarren commented Feb 4, 2019 via email

@IshaGupta18
Copy link
Collaborator

Hey @jywarren how should I test my changes that I will make to make the email responsive?

@IshaGupta18
Copy link
Collaborator

@jywarren @ViditChitkara Images are working fine to some extent, like if you change your gmail
settings->images-> "always show", some of them show up, but not all, like the PL logo and default profile picture.

@jywarren
Copy link
Member Author

jywarren commented Feb 6, 2019 via email

@IshaGupta18
Copy link
Collaborator

Hey sorry to bother you, I actually found a template in the /test/mailers/preview folder and I am trying to work on that! Thanks a lot for your help though!

@IshaGupta18
Copy link
Collaborator

I also wanted to let you know @jywarren that I will be having my mid semester exams during early mid-feb so I may not be as active, but I'll try to stay in touch as much as I can!

@jywarren
Copy link
Member Author

jywarren commented Feb 6, 2019 via email

@IshaGupta18
Copy link
Collaborator

Okay so I think I may have found a way to get rid of the page width problems, however, I am not too sure that if it will be considered as a very good way because it will require some refactoring of the existing code. Should I make a PR for it @jywarren ?

@jywarren
Copy link
Member Author

jywarren commented Feb 7, 2019 via email

@IshaGupta18 IshaGupta18 mentioned this issue Feb 8, 2019
Merged
@IshaGupta18
Copy link
Collaborator

#4780 is my PR in an attempt to fix width issues. @jywarren kindly have a look at it and let me know if there's anything else that can be done! Thanks a lot!

@arungoel123456
Copy link
Contributor

what all issues are solved ? Can u please tell

@jywarren
Copy link
Member Author

Daily/weekly fix is live! Checked it off.

image

I noticed not all images are loading so I added a new item to the checklist above.

The image URLs were:

profile pic: http:///system/profile/photos/1/thumb/bio-jeff-warren.jpg

post picture: http:///system/images/photos/000/030/324/medium/IMG_20190320_162254_076.jpg

It looks like we're skipping the "publiclab.org" part in the template. We should be able to add in Rails.root in /app/views/mailers/...

Getting there!

@jywarren
Copy link
Member Author

Made a FTO for this! #5230

@jywarren
Copy link
Member Author

jywarren commented Apr 2, 2019

Making new ones for Markdown and for the date in the title!

#5340 and #5341 !

@jywarren jywarren mentioned this issue Apr 2, 2019
Merged
5 tasks
@jywarren jywarren mentioned this issue Apr 21, 2019
Merged
@jywarren
Copy link
Member Author

Added: looks like maybe updated wiki pages are included? For now let's just show notes (#5568)

@jywarren jywarren mentioned this issue Apr 21, 2019
Merged
@IshaGupta18
Copy link
Collaborator

Okay, so I think there are a couple of more bugs with the email digest.

image

(Like the date problem and the links at the bottom)

I am gonna work on them as well as the ones you have added here @jywarren, just some refining is needed I think, and a little more integration with the parts we have broken up this issue into.

@IshaGupta18 IshaGupta18 mentioned this issue May 1, 2019
Merged
@IshaGupta18
Copy link
Collaborator

@jywarren #5638 is my PR for the broken date issue as shown above, kindly review and let me know if its good to go. Thanks!

@IshaGupta18
Copy link
Collaborator

So I have seen that all the links (after being updated to full path) are breaking somewhere and hence, both authors' and notes' images are being broken. Could someone tell me why we need to keep the full path because by keeping the original paths, the images seem to work just fine? Thanks!

@jywarren
Copy link
Member Author

jywarren commented May 1, 2019

Whoa, that's odd because i'm seeing digests like this:

image

Which is not perfect but better than what you seem to be receiving?

@IshaGupta18
Copy link
Collaborator

Yeah, I think so, but in my mail box, the links ( the "click here" links at the bottom) and the images seem to be broken:
image

What do you think should be done?

@IshaGupta18 IshaGupta18 reopened this May 1, 2019
@IshaGupta18
Copy link
Collaborator

And also, I think we can open up an FTO for the date in the heading checkpoint, because the PR #5331 is for the path links part of this issue. Please correct me if I am wrong?

@jywarren
Copy link
Member Author

jywarren commented May 1, 2019 via email

@IshaGupta18
Copy link
Collaborator

This comes from the production right?
image

This is really weird.

@jywarren
Copy link
Member Author

Almost there now! I think we need to adjust image aspect ratio:

image

Also, i confirmed, posts are getting included day after day, so I'm not sure why but maybe it's sending for any revisions, or something?

@jywarren jywarren mentioned this issue Jun 13, 2019
Merged
@jywarren
Copy link
Member Author

OK, last thing -- this section fetches revisions by timestamp, which is why old posts are getting re-added to the digest when they are updated (i.e. their revision gets a new timestamp):

https://github.com/publiclab/plots2/blob/master/app/models/user.rb#L245-L260

I'm going to make a parameter to include revisions and default it to false.

jywarren added a commit that referenced this issue Jun 15, 2019
@jywarren
add include_revisions param to content_followed_in_period
ba43e17 
Fixes #4732
@jywarren jywarren mentioned this issue Jun 15, 2019
Merged
@jywarren
Copy link
Member Author

OK! This should be completely finished when #5907 merges. Thanks all!!!!!

jywarren added a commit that referenced this issue Jun 17, 2019
@jywarren
add include_revisions param to content_followed_in_period
02653cb 
Fixes #4732
jywarren added a commit that referenced this issue Jun 20, 2019
@jywarren
add include_revisions param to content_followed_in_period
d759d1b 
Fixes #4732
jywarren added a commit that referenced this issue Jun 20, 2019
@jywarren
add include_revisions param to content_followed_in_period (#5907)
f692eb8 
* testing tests

* Update comment_test.rb

* add include_revisions param to content_followed_in_period

Fixes #4732
sagarpreet-chadha pushed a commit that referenced this issue Jun 29, 2019
@jywarren @sagarpreet-chadha
add include_revisions param to content_followed_in_period (#5907)
a99d4cd 
* testing tests

* Update comment_test.rb

* add include_revisions param to content_followed_in_period

Fixes #4732
enviro3 pushed a commit to enviro3/plots2 that referenced this issue Aug 15, 2019
@jywarren @enviro3
add include_revisions param to content_followed_in_period (publiclab#…
03ccee1 
…5907)

* testing tests

* Update comment_test.rb

* add include_revisions param to content_followed_in_period

Fixes publiclab#4732
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration help wanted requires help by anyone willing to contribute HTML planning Planning issues!
Projects
None yet
Milestone
No milestone
5 participants
@jywarren @grvsachdeva @ViditChitkara @arungoel123456 @IshaGupta18