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

in analytics, add an export button above the links graph #660

Closed
wants to merge 9 commits into from

Conversation

mrhydejc
Copy link

On analytics page, add an export button above the links graph to export links's clicks details. #650
This information is not otherwise available.
Tested on my cluster.
This is my first experience in golang and vuejs. I hope I have followed the rules.

@mrhydejc
Copy link
Author

mrhydejc commented Jan 12, 2022

it's look like this.
image

@knadh
Copy link
Owner

knadh commented Jan 13, 2022

Thanks for the PR @mrhydejc

This is my first experience in golang and vuejs. I hope I have followed the rules.

I took a quick look and it looks like a surprisingly good PR for a first time Go / Vue commit! Will review properly over the weekend and submit comments.

@knadh knadh added the enhancement New feature or request label Jan 13, 2022
@mrhydejc
Copy link
Author

Thanks @knadh
I plan to make export for the others analtytics categories (view, click, bounce).
Would you like I push them with this PR or with separates PR.

About i18n, I duplicated the key subscribers.export to campaigns.analytics.export.
Shouldn’t I be merging them in a new key globals.buttons.export ? (Be careful of language packs)

@knadh
Copy link
Owner

knadh commented Jan 14, 2022

Since it's one feature on the same page, it can be in the same PR. Yep, it should ideally be moved to globals.buttons.export.

@mrhydejc
Copy link
Author

Okay, I’m done, you can review.
I am not satisfied with campaigns.go/handleGetCampaignAnalyticsExport().
I tryed to avoid to duplicate the main for loop but the solution I found use reflect. It's look like bad design and I like simple code.
Is there a better way to achieve this with golang? If not, I can refactor with 4 differents functions.

@knadh
Copy link
Owner

knadh commented Jan 15, 2022

  • handleGetCampaignAnalyticsExport() duplicates much of what handleGetCampaignViewAnalytics() already does. Querying DB for different types of stats, scanning results to a struct, sanitizing ... A better way would be to move this logic to a shared getCampaignAnalyticsData() or something and reuse that in the current JSON handler and in the new CSV export handler. Instead of adding new SQL queries, the existing queries can be reused (with some modifications to include any new fields required by the CSV export).

  • campCountStats can be renamed to something a bit more generic like campStatsData, and the additional fields required by the CSV export can also be added there. Even if these new fields show up in the JSON API, it should be fine, but these fields can be selectively turned-off for JSON export by using json:"omitempty". This will eliminate a lot of type selection logic and reflection.

  • The bounce export seems to be exporting subscriber data and not just aggregates. I think that's more appropriate on the Subscribers -> Bounces page.

@mrhydejc
Copy link
Author

mrhydejc commented Jan 17, 2022

Hi @knadh, thanks you for the review.
Using single model with json:"omitempty" is a better approach !

Actually, my initial need is to export subscribers views and clicks with their timestamp.
As "Subscribers > Views" and "Subscribers > Clicks" pages does not exists, I thought it would be nice if the statistics CSV exports were not aggregated. It meets my need. I did the other ones (click + bounce) to make the feature consistent. (one export for each graph).
ATM, each export is not aggregated except click. (otherwise it would be the same as link's export)

With exports not aggregated, it could be difficult to share SQL as :

  • There is GROUP BY for graphs and not for the CSV
  • CSV's queries use OFFSET/LIMIT as there is lot more records.
    It’s probably could be done, but should we?

Do you want exports on statistics page to be aggregated (same datas as the graph) ?
If yes, should I :

  • Update export on analytics pages with aggregated queries
  • Create "Subscribers > Views" page and put the not-aggregated export on it (new PR)
  • Create "Subscribers > Clicks" page and put the not-aggregated export on it (new PR)
  • Move the not-aggregated Bounce export on "Subscribers > Bounces" (new PR)

@knadh
Copy link
Owner

knadh commented Jan 31, 2022

Apologies for the super delayed response @mrhydejc. Wasn't able to find time to review the big PR in detail.

Just took a close look at this and also ran it locally on a large DB. It's almost there! Couple things:

  • 2022/01/31 21:52:39 campaigns.go:735: error executing query: sql: Scan error on column index 2, name "suscriber_email": converting NULL to string is unsupported (when exporting clicks data)
  • Could you resend the PR from a feature branch from your repo instead of master? That way, any final (minor) cosmetic tweaks I'd like to add to the PR can be merged alongside your commits.

@mrhydejc
Copy link
Author

mrhydejc commented Feb 3, 2022

continue on #689

@mrhydejc mrhydejc closed this Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants