google / ioweb2016

I/O web app 2016
https://events.google.com/io2016
Apache License 2.0
517 stars 87 forks source link

Rated sessions do not display as such when revisited. #1006

Open tomconnell-wf opened 8 years ago

tomconnell-wf commented 8 years ago

I suspect that this starts happening when I get a new service worker.

Steps:

  1. Go to a session page.
  2. Rate the session.
  3. Notice that the Rate Session button is grayed out, and cannot be clicked. I believe the text changes in the button, also.
  4. Close that chrome tab.
  5. Return to that session in a new tab, the rate session button is clickable again.
  6. If you attempt to submit new feedback, you get a error message.

I did this on a Nexus 6p, mobile web.

ebidel commented 8 years ago

We're aware of this issue. For some reason the session ratings aren't being stored in FB.

@pengying

pengying commented 8 years ago

@crhym3 @nicolasgarnier

We debugged it and it seems like the backend isn't updating firebase.

x1ddos commented 8 years ago

The backend is updating firebase. otherwise, you wouldn't be able to complete steps 1-3.

pengying commented 8 years ago

Only occurs in prod :/. On May 20, 2016 11:17 PM, "alex" notifications@github.com wrote:

The backend is updating firebase. otherwise, you wouldn't be able to complete steps 1-3.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/GoogleChrome/ioweb2016/issues/1006#issuecomment-220761399

pengying commented 8 years ago

Sorry for being short at the after party. I meant it works fine in stage. But is only broken in prod.

x1ddos commented 8 years ago

So, in other words firebase security rules don't match. Is that what you're saying?

ebidel commented 8 years ago

Alex we need help debugging. Can you take a look?

On Sat, May 21, 2016, 1:56 AM alex notifications@github.com wrote:

So, in other words firebase security rules don't match. Is that what you're saying?

— You are receiving this because you commented.

Reply to this email directly or view it on GitHub https://github.com/GoogleChrome/ioweb2016/issues/1006#issuecomment-220766978

x1ddos commented 8 years ago

There's not much I see from the backend, just this:

401 Unauthorized
{"error" : "Permission denied"}
pengying commented 8 years ago

@nicolasgarnier can you take a look at the permissions?

nicolasgarnier commented 8 years ago

The permissions are the same on prod/dev/stage, I deployed the same rules automatically on stage/prod/dev typically and we deploy the same file.

You'll get a permission denied if the session has already marked as reviewed and you try to write to it though, as designed.

@crhym3 what were you trying to do when you got the error? Read or Write? Or did you just see this from the logs? I'm guessing you are seeing this error from the logs, it must be coming from a tentative write on an already submitted session which is the intended behavior. That's how we check for already submitted sessions.

nicolasgarnier commented 8 years ago

For now it seems that the data is indeed being written by the backend correctly. Peng you can check if the data is there by using the admin console for the user you are testing with:

https://io-2016-prod-<SHARD_NUM>.firebaseio.com/data/<UID>

If the data is there then this is likely just a UI issue.

pengying commented 8 years ago

Can you share the Firebase with me or check https://io-2016-prod-4.firebaseio.com/data/google:115132759088093329854

app.savedSurveys is always empty for me. IOWA.Elements.Template.app.savedSurveys []

ebidel commented 8 years ago

@pengying @nicolasgarnier , nothing is being saved:

screen shot 2016-05-23 at 4 50 57 pm
nicolasgarnier commented 8 years ago

@pengying I shared all the prod DB with you

nicolasgarnier commented 8 years ago

I think i found what the issue is:

https://io-2016-prod-9.firebaseio.com/data/google:115132759088093329854

image

Basically it seems the Backend is not writing to the same shard as the frontend. Something must be off in the Shard calculation the backend does. I could check the code if you point me to it.

Actually I found it: https://github.com/GoogleChrome/ioweb2016/blob/491d5f92629020b921739ea15a049606a8afacea/backend/survey.go#L67

nicolasgarnier commented 8 years ago

I checked the Go backend code that calculates the shard and it seems correct:

https://play.golang.org/p/TQRUZAzvwP => returns 3 (which is good since that gives us shard io-2016-prod-4.firebaseio.com)

@pengying can you double check that the UID is passed correctly as a formValue to the backend? with no extra trailing/leading characters etc...? Maybe ideally could you print out the HTTP request done to the server?

nicolasgarnier commented 8 years ago

Seems like the UID should always be passed correctly: https://github.com/GoogleChrome/ioweb2016/blob/master/app/scripts/helper/schedule.js#L308-L312

nicolasgarnier commented 8 years ago

I think I got it:

The issue is actually with the Go code and specifically at this line:

https://github.com/GoogleChrome/ioweb2016/blob/master/backend/survey.go#L68

gid := strings.TrimPrefix("google:", uid)

should instead be:

gid := strings.TrimPrefix(uid, "google:")

You can try this here: https://play.golang.org/p/DD4ZDSHuZO

gid := strings.TrimPrefix("google:", "google:115132759088093329854") prints: google: instead of 115132759088093329854.

Basically right now all survey feedback are being written to shard 9 because that's what our sharding function returns for "google:"

nicolasgarnier commented 8 years ago

We could fix it on the backend but I think it's basically a bit late to do that as we'd loose the data... Instead we could also just have the client look at shard 9 always for all session feedback data since it's all there. I don't think the load is going to be an issue now that IO has passed.

ebidel commented 8 years ago

Is the client always going to have access to shard 9?

On Tue, May 24, 2016, 6:16 AM Nicolas Garnier notifications@github.com wrote:

we coudl fix it on the backend but I think it;s basically a bit late to do that :D Instead we could also just have the client look at shard 9 always for all session feedback data since it;s all there. i don't think the load is going to be an issue now that IO has passed.

— You are receiving this because you commented.

Reply to this email directly or view it on GitHub https://github.com/GoogleChrome/ioweb2016/issues/1006#issuecomment-221264953

nicolasgarnier commented 8 years ago

yes we only restrict based on UID in the path which in this case match...

ebidel commented 8 years ago

This isn't huge priority now that IO is over, but that seems like the most logical thing to do atm now that the data is all in that location :\

@pengying if you want to fix this, I'm supportive :)

pengying commented 8 years ago

Haha. I mean I think we'll reuse the infrastructure for next year right? I think we should fix the backend and push out a version of the front end that looks at shard 9. But as part of the repo we should probably keep the non jacked up versions.

ebidel commented 8 years ago

I like that plan

On Wed, May 25, 2016, 1:08 PM Peng Ying notifications@github.com wrote:

Haha. I mean I think we'll reuse the infrastructure for next year right? I think we should fix the backend and push out a version of the front end that looks at shard 9. But as part of the repo we should probably keep the non jacked up versions.

— You are receiving this because you commented.

Reply to this email directly or view it on GitHub https://github.com/GoogleChrome/ioweb2016/issues/1006#issuecomment-221734399