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

#245 fix for Newsfeed "complete first task for today" #257

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Arilas
Copy link
Contributor

@Arilas Arilas commented Sep 11, 2015

Depends on GrooveOtter/grooveotter-api#24 because previous notifications created without task.

@L8D L8D deployed to grooveotter-demo-pr-257 September 11, 2015 12:45 Active
@Arilas
Copy link
Contributor Author

Arilas commented Sep 11, 2015

Also, previous Notifications about 'first_task' we cannot like, because it's doesn't related to any task

@garrethdev
Copy link
Member

Bug still happening on branch. I updated the server side too, and still no luck. It seems to be the same issue.
screen shot 2015-09-11 at 10 57 23 am

@Arilas
Copy link
Contributor Author

Arilas commented Sep 11, 2015

As I said, "like" will work only for new Notifications, old ones doesn't have task, so like will not work for it. We can remove old notifications with type 'notification'

@garrethdev
Copy link
Member

Yea this is a new notification. I accessed the heroku postgres instance and deleted all Notifications before I tested this and created a new one.

@Arilas
Copy link
Contributor Author

Arilas commented Sep 11, 2015

Is this instance contains changes from this: GrooveOtter/grooveotter-api#24 ? Because only after this change, it's should work. Can you see table notifications in DB, task_id should be populated in all notifications?

@garrethdev
Copy link
Member

@Arilas yea that api branch is already merged into master. You should be able to test this by deleting Notifications, and removing the Cookie TaskDay on the Client side. To access the DB:
1. Run heroku pg:psql --app whispering-scrubland-7949
2. Run DELETE FROM notifications WHERE type = 'notification';

From there you would just need to complete a few tasks to create a notification or just Post to the API as a notification

@Arilas Arilas mentioned this pull request Sep 14, 2015
@Arilas
Copy link
Contributor Author

Arilas commented Sep 14, 2015

I've checked again and it's works on local machine. And also I see that from server side going list with empty task_id column. Can you please update instance? Because as I see, pull request to API side is merged after creation of this instance

@garrethdev
Copy link
Member

@Arilas Im not quite clear what besides this needs to be merged into master?
GrooveOtter/grooveotter-api#24

@Arilas
Copy link
Contributor Author

Arilas commented Sep 14, 2015

As I see, Client send 'task_id' to server, but server doesn't save it(You can look at db, new notifications doesn't have this field). This bug was fixed in GrooveOtter/grooveotter-api#24 that was successfully merged. But as I see, it's was merged after this heroku inastance was created, so it's doesn't have this changes.

@garrethdev
Copy link
Member

@Arilas If you do a heroku restart or clear cache that should fix it. Or if you redeploy.

@L8D L8D deployed to grooveotter-demo-pr-257 September 14, 2015 16:14 Active
@garrethdev
Copy link
Member

screen shot 2015-09-15 at 7 58 16 am

After reloading the page
screen shot 2015-09-15 at 8 01 18 am

@Arilas
Copy link
Contributor Author

Arilas commented Sep 15, 2015

@GarrethDottin Is this instance uses the same database as live? Because If yes, changes doesn't work because notifications is populated from live site, that doesn't contains task_id field, because it's client side doesn't send it to server.

@garrethdev
Copy link
Member

@Arilas The client side that was testing this was done through the PR request. So any information was sent through a client side that should have the latest changes. If we're still having issues, we should probably review with Tenor @L8D as we talked about before.

Also when I was testing this morning, it was a case of me using the PR environment.

@Arilas
Copy link
Contributor Author

Arilas commented Sep 15, 2015

@Ryan-Parker @GarrethDottin Fixed. Notifications(created in this instance) now Like successfully

@garrethdev
Copy link
Member

@Arilas Still Seeing this I created it under the pr request.
screen shot 2015-09-15 at 2 25 10 pm

@garrethdev
Copy link
Member

If you want to test this on the environment I would recommend going into resources in the console and deleting the taskDay in localStorage. From there just start the timer and hit complete timer a bunch of times. This will eventually create a notification

@L8D L8D deployed to grooveotter-demo-pr-257 September 17, 2015 18:46 Active
@Arilas
Copy link
Contributor Author

Arilas commented Sep 17, 2015

@Ryan-Parker fixed this. Please run task for at least 5 seconds before click on Complete.

@L8D L8D requested a deployment to gotr-staging-pr-257 September 22, 2015 01:40 Pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants