Fix crash of the stats page with goals#91
Fix crash of the stats page with goals#91edarchis wants to merge 1 commit intovinceanalytics:mainfrom
Conversation
See issue vinceanalytics#77 for details but showing stats for goals crashed the page. The issue was that the goal events don't have a .name, only a .event. And trimUrl started with .length... This commit fixes the crash but the link for the details is still pointing to .../?filters=((is,goal,(null))) The trimUrl is also now shielded and logging about these errors.
gernest
left a comment
There was a problem hiding this comment.
@edarchis thanks for taking a stab at this.
I didn't find a previous PR with updates to the Javascript and since the JS artefacts seem to be manually generated, I included them in a separate commit
No problem, can you please remove the generated artefact and only leave the js changes so I can review the code ? I will help with updating the generated js separately when releasing the fix.
That's because the event doesn't have a .name, only .event. I supported both in list.js and shielded trimUrl that was starting with url.length...
Can you please expand what you mean by this? Does the event come from the backend? We probably need to know if it is backend of frontend issue.
I also removed some deprecated .substr() in the same file
awesome, however it will be nice if only changes fixing the issue be included here, you can always send unreleated changes in a separate PR.
|
Sure, I updated my branch to include only the minimal JS fix. You should be able to review just that. The .substr probably isn't worth a PR of its own, maybe one day if I feel like going the whole codebase to chase all of them. It was just my IDE warning about it. Regarding the issue itself, when I visit the page, I have a first event: {"n":"pageview","u":"https://www.xxx.com/","d":"xxx.com","r":null}Then when I click the button with the goal: {"n":"Homepage_button","u":"https://www.xxx.com/","d":"xxx.com","r":null,"p":{"url":"https://app.xxx.com/secure/register"}}Seems fair. The api request on the stats page is then: yielding: {"results":[{"conversion_rate":6,"event":"Homepage_button","events":0,"visitors":1}]}The issue might be that the backend doesn't return a name in the events/conversions but I'm not sure and it wasn't immediately obvious to me when reading the Go code. @gernest, some opinion on this would help avoid wasting time in the wrong direction. Thanks. |
|
Thanks , I will take a look tomorrow. I want to see if we can resolve this on the backend first before accepting this patch, since the issue is very clear here. Goals can be tied to event name or path, still need to investigate further in case this can be triggered by path as well which will mean something is broken with the backend. |
|
Any update on this? |
|
There is indeed a backend issue over here but I didn't have time to dig into it and try to make the proper fix. |
|
The same. The issue could be prevented by utilizing Typescript for the frontend. Also, I would rewrite the API usage in the dashboard to the graphql via I would work on such an update myself if @gernest would merge it. |
|
Same issue here - @gernest can you please merge und publish a new release? Thanks a lot! |
|
I just can't find time to look into this. From analysis I highly think it is a backend issue. I can't just merge this until I rule out that it is a frontend issue (which is highly unlikely) as I already explained how goals are handled. This PR is a very good starting point and a work around, however I can't just merge it without proper review since I will be the one maintaining it. |
|
@edarchis Even after I forked (https://github.com/l-you/vince) and fixed the issue, the server was still randomly crashing. So, I tired of it and created my own analytics written in GO. With strict GraphQL APIs and Typescript to avoid type errors during runtime. I would like to have some feedback about it: https://github.com/RevoTale/lovely-eye |
See issue #77 for details but in a nutshell:
That's because the event doesn't have a .name, only .event. I supported both in list.js and shielded trimUrl that was starting with url.length...
I didn't find a previous PR with updates to the Javascript and since the JS artefacts seem to be manually generated, I included them in a separate commit. Feel free to exclude it, cherry pick the rest and regenerate it yourself, or ask me to remove it if necessary. I know that I didn't (intentionally) include any malicious code in there but I wouldn't trust a stranger to include minified JS code like that in my projects. Maybe adding a Github action would be useful ?
I also removed some deprecated .substr() in the same file.