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

cherry-pick first batch of changes from main #5884

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from

Conversation

goapunk
Copy link
Contributor

@goapunk goapunk commented Dec 12, 2024

Describe your changes
I cherry-picked the changes from main we want to bring to dev, these are mostly manual changes, I will open a second PR for dependency updates.

I'll just tag everyone involved here and maybe you can have a quick look at the list of commits of concern to you and say if any of them shouldn't be included here.

@vellip wasn't sure about select_dropdown.scss: remove confusing greyed out first select item but seemed reasonable to have that on dev as well?

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

goapunk and others added 12 commits December 12, 2024 10:19
- refactor ParagraphForm.jsx to new django-ckeditor-5 api
- update django-ckeditor-5
- add setting to limit file uploads to 5mb
- extend logic from projects to external projects and plans
- add signal to reset cache when new phases are created
- add and update tests for some more cases
- improve queries with some selecte_related / prefetch_related
- use delay_on_commit for tasks called via signals to prevent race
  condition
- update celery to 5.4.0
- add module publish and unpublish signals to refresh the cache
- add project_component_updated signal to refresh the cache

fixes #5684
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.

python picks LGTM!

@vellip
Copy link
Collaborator

vellip commented Jan 7, 2025

I'd rather let @hom3mad3 have a look at the FE parts, as I haven't worked on these.

Regarding

@vellip wasn't sure about select_dropdown.scss: remove confusing greyed out first select item but seemed reasonable to have that on dev as well?

I think it doesn't matter too much to have that in because the style changed for dev anyway and I am not even sure it's being applied in dev because the structure of the SCSS changed. But it also shouldn't cause any harm to pull it in.

@goapunk
Copy link
Contributor Author

goapunk commented Jan 7, 2025

I'd rather let @hom3mad3 have a look at the FE parts, as I haven't worked on these.

Regarding

@vellip wasn't sure about select_dropdown.scss: remove confusing greyed out first select item but seemed reasonable to have that on dev as well?

I think it doesn't matter too much to have that in because the style changed for dev anyway and I am not even sure it's being applied in dev because the structure of the SCSS changed. But it also shouldn't cause any harm to pull it in.

Thanks, I'll take it out then!

@goapunk goapunk force-pushed the jd-2024-12-cherry-pick-main branch from c9adb43 to c893184 Compare January 7, 2025 08:36
@goapunk
Copy link
Contributor Author

goapunk commented Jan 7, 2025

I'll go ahead and merge this so we can update a4 again. @hom3mad3 if you have any remarks I can fix them later!

@goapunk goapunk enabled auto-merge (rebase) January 7, 2025 08:41
@goapunk goapunk disabled auto-merge January 7, 2025 09:11
@goapunk
Copy link
Contributor Author

goapunk commented Jan 7, 2025

@m4ra hmm, now the tests are failing but locally it works both on sqlite3 and postgresql, no idea what's wrong :/

@m4ra
Copy link
Contributor

m4ra commented Jan 7, 2025

Here same, tests pass both in sqlite3 and postgresql. I re-run the build.

@m4ra
Copy link
Contributor

m4ra commented Jan 7, 2025

Same error. I don't remember why we have set 3 queries when requesting the plans. Do you? @goapunk

@goapunk
Copy link
Contributor Author

goapunk commented Jan 8, 2025

Same error. I don't remember why we have set 3 queries when requesting the plans. Do you? @goapunk

not really, I think that was just the number when we first ran the test. The confusing thing is it works on main, so I don't really know what's different on dev which breaks it? I will try to have a closer look at the queries

@goapunk goapunk mentioned this pull request Jan 8, 2025
5 tasks
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