-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add talk-component to dashboard #272
Conversation
Co-authored-by: Mario Behling <[email protected]>
* Update Stripe to more recent version
* upgrade vue version from 2.6.12 -> 2.7.14 * load vue.min.js file for prod env and vue.js for debug purposes * set default value to empty string in select field --------- Co-authored-by: Mario Behling <[email protected]>
* pump python to version 3.11 * Update pyproject.toml with correct Django version --------- Co-authored-by: Mario Behling <[email protected]> Co-authored-by: Norbert Preining <[email protected]>
* Update Stripe to more recent version
* Fix Badge API documentation error * Added Default Values for Size
* Update Stripe to more recent version
* upgrade vue version from 2.6.12 -> 2.7.14 * load vue.min.js file for prod env and vue.js for debug purposes * set default value to empty string in select field --------- Co-authored-by: Mario Behling <[email protected]>
* pump python to version 3.11 * Update pyproject.toml with correct Django version --------- Co-authored-by: Mario Behling <[email protected]> Co-authored-by: Norbert Preining <[email protected]>
* Update Stripe to more recent version
* Fix Badge API documentation error * Added Default Values for Size
* correct path for stripe plugin template
Update master with latest development changes.
* update text + fields position * added global setting for email, update function get_mail_backend * added text field for ticket question form * align left for text field
* add support Ukrainan * change Ukrainian langcode from uk-UA to uk * update css to show ukraina flag
Updating master with latest development changes.
* set required to False for question with type is DES, remove question with type DES out of cart
* remove support for Mariadb/MySQL
* Improve flow for required field of Yes-No question
Co-authored-by: Mario Behling <[email protected]>
* Add plugins to autodeployment with docker
* Implement option to add link to privacy policy
* add footer link for event + organizer
* Replace pkg_resources with import importlib_metadata
* Do not repeat questions in form field of ticket form
* implement customer feature * implement customer SSO provider & client * add docs for customer functions
* fix error when create an event from another event
* integrate with SendGrid * add dependency for sendgrid * revert gitignore * added global setting for email, update function get_mail_backend * load default setting from .cfg file
* Use master branch in docker-pr workflow * Update workflow test postgres version to 12
Reviewer's Guide by SourceryThis pull request introduces a new talk-component to the dashboard by updating the layout and styling of the authentication and presale pages, adding new sections for better user experience, and implementing a new ComponentsView with a corresponding URL pattern and template to display the components page. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @AviGawande - I've reviewed your changes - here's some feedback:
Overall Comments:
- The ComponentsView function in auth.py uses a hardcoded file path. This should be replaced with a configurable setting to ensure the code works across different environments.
- The addition of ComponentsView in auth.py seems to mix concerns. Consider moving this functionality to a more appropriate location in the codebase.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -439,3 +439,17 @@ def get_context_data(self, **kwargs): | |||
|
|||
def get(self, request, *args, **kwargs): | |||
return super().get(request, *args, **kwargs) | |||
|
|||
|
|||
def ComponentsView(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 suggestion (security): Consider using a configuration variable for the parent directory path
The hardcoded path '/home/abhigawande/Downloads/fossasia' is not portable and could lead to security issues. Consider using a configuration variable or environment variable for this path.
def ComponentsView(request):
parent_directory = settings.FOSSASIA_DIRECTORY
{% endblocktrans %} | ||
</p> | ||
<p> | ||
{% trans "This is a self-hosted installation of" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Optimize repeated translated strings
There's repetition of translated strings. Consider using a single translated string for repeated content to improve maintainability.
{% trans "This is a self-hosted installation of" %} | |
{% blocktrans trimmed %} | |
This is a self-hosted installation of <a href="https://eventyay.com">eventyay, your open source event solution</a>. | |
{% endblocktrans %} |
|
||
{% if is_eventyay_talk_installed %} | ||
<p> | ||
<a href="http://127.0.0.1:8000/eventyay-talk" class="btn btn-primary"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the hostname hardcoded as "127.0.0.1:8000"? It will not work in live system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had put this while testing,i had corrected it now.
src/pretix/control/urls.py
Outdated
@@ -16,6 +16,7 @@ | |||
url(r'^invite/(?P<token>[a-zA-Z0-9]+)$', auth.invite, name='auth.invite'), | |||
url(r'^forgot$', auth.Forgot.as_view(), name='auth.forgot'), | |||
url(r'^forgot/recover$', auth.Recover.as_view(), name='auth.forgot.recover'), | |||
url(r'^component$',auth.ComponentsView, name='auth.component'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a whitespace after ,
.
src/pretix/control/views/auth.py
Outdated
'eventyay_talk_path': eventyay_talk_path if is_eventyay_talk_installed else None, | ||
} | ||
|
||
return render(request, 'pretixcontrol/auth/components.html', context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add one blank line at the end of file and configure your IDE to automatically do so,
<h2>{% trans "WELCOME TO EVENTYAY-TICKETS" %}</h2> | ||
<p> | ||
{% blocktrans trimmed with a_attr="href='https://eventyay.com'" %} | ||
This is a self-hosted installation of <a {{ a_attr }}>eventyay, your open source event solution.</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bad practice to put code (like <a>
, </a>
) in to-be-translated string. Because those strings will be handovered to translators, who are not tech persons and will likely put wrong code, which will break our site when the translated strings are applied.
But this mistake is from the original pretalx code. It is not urgent to fix right now.
d832dd9
to
80c867c
Compare
This PR fixes #257
Attached Screenshot
Summary by Sourcery
Add a new 'Components' page to the dashboard to check the installation status of 'eventyay-talk'. Revamp the layout and styling of the authentication base template and update the index page with additional information and resources.
New Features:
Enhancements: