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

GT-1314, implement saving user count values #826

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

Conversation

andrewroth
Copy link
Collaborator

No description provided.

db/schema.rb Outdated Show resolved Hide resolved
@stage-branch-merger
Copy link

I see you added the "On Staging" label, I'll get this merged to the staging branch!

@stage-branch-merger
Copy link

Merge conflict attempting to merge this into staging. Please fix manually.

@frett frett force-pushed the GT-1314-record-counter-values branch from 4640b2c to fba04d8 Compare March 29, 2022 13:09
db/migrate/20211201161437_create_user_counter_values.rb Outdated Show resolved Hide resolved
app/controllers/user_counters_controller.rb Outdated Show resolved Hide resolved
db/migrate/20211201161437_create_user_counter_values.rb Outdated Show resolved Hide resolved
app/controllers/user_counters_controller.rb Outdated Show resolved Hide resolved
@andrewroth andrewroth requested a review from knutsenm May 4, 2022 21:28
Copy link
Contributor

@knutsenm knutsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, rubocop has a couple tiny complaints

spec/acceptance/user_counters_controller_spec.rb Outdated Show resolved Hide resolved
db/schema.rb Outdated Show resolved Hide resolved
@frett
Copy link
Contributor

frett commented May 5, 2022

Don't merge this yet, I want to wait on merging this until we have a tangible use-case for it. Until then it's fine remaining approved and open.

db/schema.rb Outdated
Comment on lines 261 to 278
add_foreign_key "attachments", "resources"
add_foreign_key "attributes", "resources"
add_foreign_key "auth_tokens", "access_codes"
add_foreign_key "custom_manifests", "languages"
add_foreign_key "custom_manifests", "resources"
add_foreign_key "custom_pages", "languages"
add_foreign_key "custom_pages", "pages"
add_foreign_key "follow_ups", "destinations"
add_foreign_key "follow_ups", "languages"
add_foreign_key "pages", "resources"
add_foreign_key "resources", "resource_types"
add_foreign_key "resources", "systems"
add_foreign_key "translated_attributes", "attributes"
add_foreign_key "translated_attributes", "translations"
add_foreign_key "translated_pages", "languages"
add_foreign_key "translated_pages", "resources"
add_foreign_key "translations", "languages"
add_foreign_key "translations", "resources"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you dropped all the foreign keys when converting the values to an array field

@stage-branch-merger
Copy link

Merge conflict attempting to merge this into staging. Please fix manually.

@frett frett removed the On Staging label Sep 9, 2022
@frett
Copy link
Contributor

frett commented Nov 9, 2022

@andrewroth @knutsenm I finally found a use case for this, so we can go ahead and fix the merge conflicts and get this merged into production.

Hmm, maybe not after thinking a bit deeper on possible use cases for the relevant counters going forward. It could be useful to keep track of how often specific languages or specific articles are opened to support future features of "most used languages" or "most read articles"

Copy link
Contributor

@knutsenm knutsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong review request?

Github says you have to resolve some merge conflicts here.

@andrewroth
Copy link
Collaborator Author

Yeah, my bad. Wanted #1228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants