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

Feature/upgrade django version #341

Closed
wants to merge 1 commit into from

Conversation

ErhanCitil
Copy link
Contributor

No description provided.

@@ -8,31 +8,33 @@ python-decouple # processing of envvar configs
jsonschema

# Framework libraries
django < 3.0
django==3.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove all pinned versions from base.in file. We pin dependencies in base.txt

package.json Outdated
@@ -20,6 +20,7 @@
"homepage": "https://maykinmedia.nl",
"dependencies": {
"microscope-sass": "latest",
"prettier": "^2.8.8",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you added prettier?

@@ -8,7 +8,6 @@


class Migration(migrations.Migration):

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please in the future make a separate PR for linting. And feel free to merge it, because it doesn't affect the code
Now it's very hard to review


from .models import User


@admin.register(User)
class _UserAdmin(UserAdmin, HijackUserAdminMixin):
list_display = UserAdmin.list_display + ("hijack_field",)
list_display = UserAdmin.list_display + ("is_active", "is_staff", "is_superuser")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to add these fields in the list view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No there is no reason, I can remove it if it's not necessary

@@ -73,7 +73,7 @@
"sniplates",
"hijack",
"compat", # Part of hijack
"hijack_admin",
# "hijack_admin",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the line it if it's not used

Comment on lines +114 to +113
"vng_api_common.middleware.AuthMiddleware",
"vng_api_common.middleware.APIVersionHeaderMiddleware",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are they added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the documentation it says that I need to add these middleware:

Documentation

AXES_USE_USER_AGENT = True # Default: False
AXES_LOCK_OUT_BY_COMBINATION_USER_AND_IP = True # Default: False
AXES_LOCKOUT_PARAMETERS = [["ip_address", "user_agent"]]
AXES_LOCKOUT_PARAMETERS = [["username", "ip_address"]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two lines for one parameter

# If set, specifies a template to render when a user is locked out. Template
# receives cooloff_time and failure_limit as context variables. Default: None
AXES_LOCKOUT_TEMPLATE = "account_blocked.html"
AXES_USE_USER_AGENT = True # Default: False
AXES_LOCK_OUT_BY_COMBINATION_USER_AND_IP = True # Default: False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you decided to change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Django Axes 6.0.0 these settings were deprecated:

Django Axes Changelog

Comment on lines 445 to 446
PRIVATE_MEDIA_ROOT = os.path.join(BASE_DIR, "private-media")
PRIVATE_MEDIA_URL = "/private-media/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need it? I don't remember that Objects API stores user documents

@ErhanCitil ErhanCitil force-pushed the feature/upgrade-django-version branch 13 times, most recently from d589089 to 7d86a57 Compare August 3, 2023 10:42
@ErhanCitil ErhanCitil force-pushed the feature/upgrade-django-version branch from 3e4af61 to e8fc6ef Compare August 3, 2023 12:11
@Viicos Viicos mentioned this pull request Jan 18, 2024
@Viicos Viicos closed this Jan 18, 2024
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.

Objects API & ObjectTypes API - Django upgrade naar minimaal 3.2
3 participants