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

Modernize django filer #1344

Closed
wants to merge 21 commits into from
Closed

Modernize django filer #1344

wants to merge 21 commits into from

Conversation

jrief
Copy link
Collaborator

@jrief jrief commented May 10, 2023

Description

superseeds #1341

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

jrief added 18 commits May 9, 2023 16:25
Replace re_path against path
Use admin.decorators.action to mark actions
Remove dysfunctional jQuery function $.actions()
The Django-Admin loads a JS file named admin/js/actions.js for all list views. It handles the action button on top. Since django-filer handles its actions itself, that button must be mimicked, otherwise a JS error is thrown, complaining that this button can not be found in the DOM.
Replace re_path against path
Use admin.decorators.action to mark actions
Remove dysfunctional jQuery function $.actions()

replace History button against Django's own version

prevent JS error in Django's actions.js

The Django-Admin loads a JS file named admin/js/actions.js for all list views. It handles the action button on top. Since django-filer handles its actions itself, that button must be mimicked, otherwise a JS error is thrown, complaining that this button can not be found in the DOM.

flake8 is complaining

use proper decorator @action for admin actions

drop support for Django<3.2 and Python<3.8

wait with django-4.2 until django-app-helper is up to date

fix failing unit tests

fix indent complains

use proper decorator for templatetag

pilfer actions_selection_counter from original Django file

drop support for Django<3.2 and Python<3.8
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #1344 (120e604) into master (0bbbe88) will decrease coverage by 0.39%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1344      +/-   ##
==========================================
- Coverage   72.40%   72.02%   -0.39%     
==========================================
  Files          72       72              
  Lines        3269     3235      -34     
  Branches      532      537       +5     
==========================================
- Hits         2367     2330      -37     
- Misses        735      739       +4     
+ Partials      167      166       -1     
Impacted Files Coverage Δ
filer/admin/tools.py 76.81% <ø> (-2.68%) ⬇️
filer/settings.py 79.06% <ø> (+0.34%) ⬆️
filer/admin/folderadmin.py 71.72% <100.00%> (-0.15%) ⬇️
filer/templatetags/filer_admin_tags.py 86.95% <100.00%> (-1.66%) ⬇️

... and 2 files with indirect coverage changes

requirements-file: [
django-2.2.txt,
django-3.0.txt,
django-3.1.txt,
django-3.2.txt,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I'm almost certain django 3.2, 4.0 don't support python 3.11, so some exclusions will still be required.

@@ -706,115 +682,3 @@ body {
display: table-cell;
vertical-align: middle;
}

.filebrowser .navigator-thumbnail-list {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Are a lot of the code being removed through these files related to this new preview functionality?

As I understand it, the regression issues caused by that features mean certain elements of the UI don't work. But these changes remove so much it'll make it much harder to bring back this feature if it's completely been removed.

Is there not a less destructive way to "hide" it until it's fixed?

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 just reverted to complete pull request, ie. #1255.
It probably is easier for whomever works on it again, to integrate it into the then master branch.
The CSS here is only used for that purpose, so keeping it may introduce a conflict in the future.

setup.py Outdated
'Framework :: Django :: 3.2',
'Framework :: Django :: 4.0',
'Framework :: Django :: 4.1',
'Framework :: Django :: 4.2',
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

why add the 4.2 classifier if the install doesn't allow for 4.2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ups, my bad.

@stale
Copy link

stale bot commented Aug 9, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 9, 2023
@stale
Copy link

stale bot commented Sep 6, 2023

This will now be closed due to inactivity, but feel free to reopen it.

@stale stale bot closed this Sep 6, 2023
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.

None yet

2 participants