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

[#8875] insights: add a+ insights logic and display on project detail #6123

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

vellip
Copy link
Collaborator

@vellip vellip commented Feb 20, 2025

also adds project result page, insight logic mostly taken from a+, with some changes. Not moved into a4 for a quicker implementation

Tasks

  • PR name contains story or task reference
  • Steps to recreate and test the changes
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@vellip vellip force-pushed the pv-2025-02-insights-and-results branch 2 times, most recently from 2509b8c to d331c4b Compare February 25, 2025 08:27
@vellip vellip requested a review from goapunk February 25, 2025 08:27
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

Thanks! I left a comment regarding the ratings

@goapunk
Copy link
Contributor

goapunk commented Feb 25, 2025

@m4ra one thing we need to take into account that the insights data migration will take quite a long time, locally I got about 7min, from experience that might take up to 1h on prod

@goapunk
Copy link
Contributor

goapunk commented Feb 25, 2025

@vellip needs cherry-picking because of the force-pushed migrations to dev

@goapunk goapunk requested a review from m4ra February 25, 2025 17:30
@vellip vellip force-pushed the pv-2025-02-insights-and-results branch 2 times, most recently from ebbb69c to fe4c905 Compare February 26, 2025 13:01
@vellip
Copy link
Collaborator Author

vellip commented Feb 26, 2025

Added maptopics and a logger output when the migration is happening

@vellip vellip requested a review from goapunk February 26, 2025 13:02
@vellip vellip force-pushed the pv-2025-02-insights-and-results branch from fe4c905 to 965b634 Compare February 26, 2025 13:27
@goapunk
Copy link
Contributor

goapunk commented Feb 26, 2025

@m4ra @vellip I wonder if we should just leave the 0007 migration out. This way deploying will be much faster and we can just run the management command to populate the insights without requiring a downtime. What do you think?

@vellip
Copy link
Collaborator Author

vellip commented Feb 26, 2025

@goapunk hmm, might make your lives easier when it comes to deploying, yeaah. Should I take it out?

insights = defaultdict(ProjectInsight)
user_ids = defaultdict(set)

logger.info("creating insights")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd actually prefer not to have that logging, it doesn't really help, django already tells us what migration is currently running :)

@goapunk
Copy link
Contributor

goapunk commented Feb 27, 2025

@goapunk hmm, might make your lives easier when it comes to deploying, yeaah. Should I take it out?

Let's take it out, yeah. If we end up needing it we can still add it later

@vellip vellip force-pushed the pv-2025-02-insights-and-results branch from 965b634 to 52f6fc1 Compare February 27, 2025 10:34
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

Great! just a few things

@vellip vellip force-pushed the pv-2025-02-insights-and-results branch from 52f6fc1 to f8dda4a Compare February 27, 2025 12:00
show_live_questions = "IE" in blueprint_types
show_ideas = bool(
blueprint_types.intersection(
{"BS", "IC", "MBS", "TP", "MTP", "MIC", "PB", "PB2", "PB3"}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also added Topic Prio and Map Topic Prio here. I hope that is right?

@vellip vellip force-pushed the pv-2025-02-insights-and-results branch from f8dda4a to b9a4d76 Compare February 27, 2025 12:18
@vellip vellip requested review from goapunk and m4ra February 27, 2025 12:19
@vellip vellip force-pushed the pv-2025-02-insights-and-results branch from b9a4d76 to f032da3 Compare February 28, 2025 08:25
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.

3 participants