niryariv / opentaba-client

BSD 3-Clause "New" or "Revised" License
8 stars 15 forks source link

Email Notifier for client #111

Closed noamoss closed 8 years ago

noamoss commented 8 years ago

@niryariv @alonisser seems to work - would you mind to recheck and merge if seems to be ready? The notifier (beta) service is up and running.

niryariv commented 8 years ago

getting 502 on http://82.196.4.213/addfeed/opentaba?city=%D7%99%D7%A8%D7%95%D7%A9%D7%9C%D7%99%D7%9D&link=http://opentaba-server-jerusalem.herokuapp.com/gush/plans.atom

alonisser commented 8 years ago

Strange, I'm not getting a 502. is this on heroku? also why use hard coded IP adress and not dns?

On Sat, May 28, 2016 at 11:46 AM Nir Yariv notifications@github.com wrote:

getting 502 on http://82.196.4.213/addfeed/opentaba?city=%D7%99%D7%A8%D7%95%D7%A9%D7%9C%D7%99%D7%9D&link=http://opentaba-server-jerusalem.herokuapp.com/gush/plans.atom

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/niryariv/opentaba-client/pull/111#issuecomment-222297760, or mute the thread https://github.com/notifications/unsubscribe/ABXlcD1a9YdbNUzCFFdU_353UyGK_dt-ks5qGADsgaJpZM4In-qt .

niryariv commented 8 years ago

Having an actual domain name instead of an IP address

On Sat, May 28, 2016 at 5:50 PM, noamoss notifications@github.com wrote:

wht do you mean by using dns. ?

On Sat, May 28, 2016 at 1:51 AM -0700, "Alonisser" < notifications@github.commailto:notifications@github.com> wrote:

Strange, I'm not getting a 502. is this on heroku? also why use hard coded IP adress and not dns?

On Sat, May 28, 2016 at 11:46 AM Nir Yariv notifications@github.com wrote:

getting 502 on

http://82.196.4.213/addfeed/opentaba?city=%D7%99%D7%A8%D7%95%D7%A9%D7%9C%D7%99%D7%9D&link=http://opentaba-server-jerusalem.herokuapp.com/gush/plans.atom

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub < https://github.com/niryariv/opentaba-client/pull/111#issuecomment-222297760 , or mute the thread < https://github.com/notifications/unsubscribe/ABXlcD1a9YdbNUzCFFdU_353UyGK_dt-ks5qGADsgaJpZM4In-qt

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub< https://github.com/niryariv/opentaba-client/pull/111#issuecomment-222297959>, or mute the thread< https://github.com/notifications/unsubscribe/ADanLd5zq1u2vMWfmQL6_J219mr7E9Egks5qGAIOgaJpZM4In-qt

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/niryariv/opentaba-client/pull/111#issuecomment-222312346, or mute the thread https://github.com/notifications/unsubscribe/AAAG67MalFQdEJvzM3Ik9oVEQfM3Q5j_ks5qGFYygaJpZM4In-qt .

noamoss commented 8 years ago

I believe the 502 error @niryariv saw is a result of the way you set and ran the check (local server with nothing to serve). When referring to Heroku it works as expcted. Please recheck as I now setting API_URL var to heroku automatically.

@alonisser no domain yet. I will fix it once the notification service will be stable and active.

Yalla?

florpor commented 8 years ago

Looks good to me, no 502.

But @noamoss please revert the API_URL back as it is needed for the automated tests and they fail now... Plus, based on the url @niryariv linked it doesn't seem he was sending a local url (the herokuapp is linked). Maybe you should look into the notifier app logs and see if there was an actual problem? Probably with the host...

noamoss commented 8 years ago

@florpor reversed the API_URL and simplified the notifier_linked to appear within the rendering operation. How does it look?

florpor commented 8 years ago

looks good! just a couple of things:

  1. the tooltip for registering to notifications for a group of gushim is half hidden behind the map. you can fix it by changing the tooltip-registering line in the template file to: $("[rel='tooltip']").tooltip({'placement' : 'bottom', 'container': 'body'});
  2. the tests still fail, you should update the 'a' element count in test_index_get_gush.js file for them to run smoothly.

also - is there a place i can open an issue for the notification system (little bird?)? the font isn't working properly on firefox... (at least on linux)

noamoss commented 8 years ago

@florpor just committed with both issues - tests seems to fail, any idea why?

notifier's feedback is very welcome at https://github.com/noamoss/notifier/issues. Look for improvement soon.

noamoss commented 8 years ago

Now it's on: login/register on the fly: notifier_addfeed notifier_screenshot

florpor commented 8 years ago

travis has some issues with the map image-comparison tests for some reason... testing the same code with the same versions (casperjs + phantomjs) runs successfully on my machine. as long as it runs fine locally for you i'm ok with this. will investigate travis when i have time.

@noamoss are there plans to setup a domain name for the notifier? plus is it tested? are any other sadna projects using it already?

noamoss commented 8 years ago

@florpor runs localy fine for me. @niryariv can you approve also? Are we merging and deploying?

@florpor I guess that when we will have more than one project in out (beta) email notification center we will move into a domain name. Opentaba was the original reason for this project, therefore it is the first one to include. At the same time we are already testing it with Kikar Ha'medina. It was built to fit any project with customized feed service.

There is still some work to be done over the email template, and than, hopefully, we will wrap it into a mobile-app with a push service.

noamoss commented 8 years ago

@florpor updated app.js to a temporary domain name (we will get a normal one soon).

niryariv commented 8 years ago

@noamoss I didn't get to reviewing the whole thing yet but just to explain regarding the domain:

You'll be sending emails to users with unsubscribe links etc. If you change to a new domain, links in old emails will stop working and users will be mad at OpenTaba. So in order to deploy we have to have a final, stable domain.

noamoss commented 8 years ago

@niryariv we will create an auto redirect once we will get to a final domain name. I will also have to add a disclaimer saying it is a beta service in the experiment period.

niryariv commented 8 years ago

@noamoss why not just register a domain now and not have to worry about all that? it's 5 minutes and $10 ;)