Eventyay-tickets Integration #115
Conversation
✅ Deploy Preview for open-event-checkin ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thank you. You can show the functionality in the weekly meetings and we can discuss then if we should keep this in a seperate branch. |
|
I have added the required frontend component to select a server in the LoginForm Component Screen Shots
|
|
A "/eventyayevents" page has been added from where the events can be selected to perform checkin actions from a list of events the device has access to. 
This PR has multiple |
|
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request introduces a new feature allowing event organizers to register devices via a QR code scan interface. A 'Register Device' button has been added to the home page, and new routes and components have been implemented to support this functionality. Additionally, the login form has been enhanced with a server selection dropdown and corresponding error handling. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Sak1012 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Define submitForm method (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 9 other issues
- 🟢 Security: all looks good
- 🟢 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.
src/components/LoginForm.vue
Outdated
|
|
||
| // router | ||
| const router = useRouter() | ||
|
|
||
| async function submitLogin() { | ||
| console.log(server.value) | ||
| if (server.value === '' || server.value === 'Select a Server') { |
There was a problem hiding this comment.
suggestion: Consider using a constant for the default server value
Using a constant for the default server value ('Select a Server') can help avoid potential typos and make the code easier to maintain.
| if (server.value === '' || server.value === 'Select a Server') { | |
| const DEFAULT_SERVER_VALUE = 'Select a Server'; | |
| if (server.value === '' || server.value === DEFAULT_SERVER_VALUE) { |
src/stores/eventyayEvent.js
Outdated
|
|
||
| try { | ||
| const api = mande(url, { headers: { Authorization: `Device ${apiToken}` } }) | ||
| console.log(`/api/v1/organizers/${organizer}/events/`) |
There was a problem hiding this comment.
issue (performance): Remove console.log statements
Leaving console.log statements in production code can lead to performance issues and potential information leakage. Consider removing or replacing them with proper logging mechanisms.
src/stores/eventyayEvent.js
Outdated
| const api = mande(url, { headers: { Authorization: `Device ${apiToken}` } }) | ||
| console.log(`/api/v1/organizers/${organizer}/events/`) | ||
| const response = await api.get(`/api/v1/organizers/${organizer}/events/`) | ||
| console.log('Hello', response) |
There was a problem hiding this comment.
issue (performance): Remove console.log statements
Leaving console.log statements in production code can lead to performance issues and potential information leakage. Consider removing or replacing them with proper logging mechanisms.
| /> | ||
| </div> | ||
| </form> | ||
| <div v-if="!loading && !events.length && !error">No events available</div> |
There was a problem hiding this comment.
suggestion: Consider adding a retry mechanism
If no events are available, consider providing a retry mechanism or a more detailed message to guide the user.
| <div v-if="!loading && !events.length && !error">No events available</div> | |
| <div v-if="!loading && !events.length && !error"> | |
| No events available | |
| <button @click="fetchEvents">Retry</button> | |
| </div> |
Sak1012
force-pushed
the
badge-integration
branch
from
6834ad7
to
57f721b
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @Sak1012 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Potential hard-coded secret found in 'token' field. (link)
- Potential hard-coded secret found. (link)
Overall Comments:
- The error handling in the
authDevicefunction could be improved. Consider using more specific error types and providing more informative error messages to the user. - The hardcoded port number for localhost (
:8000) in theauthDevicefunction is not a good practice. Consider making this configurable or using a default port that doesn't require modification. - There's no input validation for the manual device key entry in the
Device.vuecomponent. Implement proper validation to ensure the entered key meets the required format and length.
Here's what I looked at during the review
- 🟡 General issues: 10 issues found
- 🔴 Security: 2 blocking issues
- 🟢 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.
src/components/LoginForm.vue
Outdated
|
|
||
| // router | ||
| const router = useRouter() | ||
|
|
||
| async function submitLogin() { | ||
| console.log(server.value) |
There was a problem hiding this comment.
suggestion: Remove console.log statements
Consider removing the console.log statements used for debugging purposes before merging the code to the main branch.
| console.log(server.value) | |
| const router = useRouter() | |
| async function submitLogin() { | |
| if (server.value === '' || server.value === 'Select a Server') { | |
| showServerError.value = true |
src/components/LoginForm.vue
Outdated
|
|
||
| // router | ||
| const router = useRouter() | ||
|
|
||
| async function submitLogin() { | ||
| console.log(server.value) | ||
| if (server.value === '' || server.value === 'Select a Server') { |
There was a problem hiding this comment.
suggestion: Use a constant for 'Select a Server'
Consider defining 'Select a Server' as a constant to avoid potential typos and make the code more maintainable.
| if (server.value === '' || server.value === 'Select a Server') { | |
| const SELECT_A_SERVER = 'Select a Server'; | |
| if (server.value === '' || server.value === SELECT_A_SERVER) { |
There was a problem hiding this comment.
Please define constants to compare server.value with.
There was a problem hiding this comment.
Absolutely, defining constants for strings like 'Select a Server' is a great practice. It helps prevent typos and makes the code more readable and maintainable. You can define the constant at the top of your script or in a separate configuration file if it's used across multiple components. Here's how you can implement it:
const SELECT_A_SERVER = 'Select a Server';
if (server.value === '' || server.value === SELECT_A_SERVER) {
// Handle the error case
}This way, if you ever need to change the placeholder text, you only have to update it in one place. Let me know if you need further assistance!
There was a problem hiding this comment.
Done, I have fixed it in the recent commit.
There was a problem hiding this comment.
Great to hear that you've implemented the change! Using constants for strings is definitely a good practice for maintaining clean and error-free code. If you have any more questions or need further assistance with anything else in the PR, feel free to ask. Keep up the good work!
| <StandardButton | ||
| :type="'button'" | ||
| :text="'Register-Device'" | ||
| :disabled="false" | ||
| class="btn-primary mt-6 w-full justify-center" | ||
| @click="registerDevice" | ||
| > | ||
| </StandardButton> |
There was a problem hiding this comment.
suggestion: Button text should be a prop
Consider passing the button text as a prop to the StandardButton component for better reusability and consistency.
| <StandardButton | |
| :type="'button'" | |
| :text="'Register-Device'" | |
| :disabled="false" | |
| class="btn-primary mt-6 w-full justify-center" | |
| @click="registerDevice" | |
| > | |
| </StandardButton> | |
| <StandardButton | |
| :type="'button'" | |
| :text="buttonText" | |
| :disabled="false" | |
| class="btn-primary mt-6 w-full justify-center" | |
| @click="registerDevice" | |
| /> |
There was a problem hiding this comment.
If you are passing a static string (Register-Device) to text prop, no point to use bind syntax and JS expression.
Just pass as normal HTML attribute like this:
<StandardButton text='Register-Device' ... />There was a problem hiding this comment.
Great point, @hongquan! If the button text is static and won't change, using a regular HTML attribute is indeed more straightforward and cleaner. However, if there's a possibility that the button text might need to be dynamic or localized in the future, passing it as a prop could be beneficial for flexibility. It really depends on the expected use case. If you foresee any changes or need for localization, consider using a prop; otherwise, your suggestion is perfectly valid for a static text.
There was a problem hiding this comment.
As this was the convention followed everywhere else where StandardButton component was previously being used, I too did the same should I still change it ?
There was a problem hiding this comment.
If the convention in your codebase is to use the binding syntax for the text prop, even for static strings, it's generally a good idea to maintain consistency with the existing code. This can help with readability and maintainability, especially if there's a chance that the text might become dynamic or require localization in the future.
However, if you and your team decide that using a regular HTML attribute for static text is more appropriate and aligns with your coding standards, you can certainly make that change. Consistency is key, so whichever approach you choose, it would be beneficial to apply it uniformly across your project.
If you're unsure, it might be worth discussing with your team to see if there's a preference or if there's a plan to make the button text dynamic in the future.
There was a problem hiding this comment.
That is not convention, previous people simply don't understand VueJS well enough.
There was a problem hiding this comment.
It sounds like there's a bit of a misunderstanding about the use of VueJS syntax in your codebase. If the current practice is to use the binding syntax for static strings, it might be due to a lack of understanding rather than a deliberate convention.
Given this, it could be a good opportunity to align on best practices with your team. Using a regular HTML attribute for static text is more straightforward and should be preferred unless there's a specific reason to use the binding syntax, such as future plans for dynamic content or localization.
If you decide to switch to using regular HTML attributes for static text, it might be helpful to update the existing code for consistency. This could also be a chance to document this as a coding standard for your team to follow moving forward. If you need further guidance, consider discussing this with your team or a lead developer to ensure everyone is on the same page.
src/components/LoginForm.vue
Outdated
| <div v-if="showServerError" class="mt-5"> | ||
| <p class="text-center text-sm text-danger">Please select a server first</p> |
There was a problem hiding this comment.
suggestion: Error message should be configurable
Consider making the error message configurable or translatable to support multiple languages.
| <div v-if="showServerError" class="mt-5"> | |
| <p class="text-center text-sm text-danger">Please select a server first</p> | |
| <div v-if="showServerError" class="mt-5"> | |
| <p class="text-center text-sm text-danger">{{ errorMessage }}</p> |
src/stores/processDevice.js
Outdated
| if (url.includes('localhost')) { | ||
| url = `${url}:8000` // Add your desired port number here |
There was a problem hiding this comment.
suggestion: Avoid hardcoding port number
Consider making the port number configurable instead of hardcoding it.
| if (url.includes('localhost')) { | |
| url = `${url}:8000` // Add your desired port number here | |
| const port = process.env.PORT || 8000; | |
| if (url.includes('localhost')) { | |
| url = `${url}:${port}` |
There was a problem hiding this comment.
Please do as sourcery-ai suggested.
There was a problem hiding this comment.
Absolutely, making the port number configurable is a good practice as it enhances flexibility and adaptability across different environments. By using process.env.PORT || 8000, you allow the application to dynamically adapt to the port specified in the environment variables, while still providing a default value of 8000 if none is set. This approach is particularly useful in development and deployment scenarios where the port might vary. If you need any help implementing this change or have further questions, feel free to ask!
There was a problem hiding this comment.
I have fixed this in the recent commit.
There was a problem hiding this comment.
Great to hear that you've implemented the change, @Sak1012! Making the port configurable is a solid improvement. If you encounter any issues or have further questions as you continue working on this feature, feel free to reach out. Keep up the good work!
src/stores/eventyayEvent.js
Outdated
|
|
||
| try { | ||
| const api = mande(url, { headers: { Authorization: `Device ${apiToken}` } }) | ||
| console.log(`/api/v1/organizers/${organizer}/events/`) |
There was a problem hiding this comment.
suggestion: Remove console.log statements
Consider removing the console.log statements used for debugging purposes before merging the code to the main branch.
| console.log(`/api/v1/organizers/${organizer}/events/`) | |
| // console.log(`/api/v1/organizers/${organizer}/events/`) |
| } | ||
|
|
||
| const payload = { | ||
| token: qrData.token, |
There was a problem hiding this comment.
🚨 issue (security): Potential hard-coded secret found in 'token' field.
The 'token' field appears to be a sensitive value. Ensure that this is not a hard-coded secret and is securely managed.
hs
Outdated
| @@ -0,0 +1 @@ | |||
| sak:3670 | |||
There was a problem hiding this comment.
🚨 issue (security): Potential hard-coded secret found.
The line 'sak:3670' appears to be a sensitive value. Ensure that this is not a hard-coded secret and is securely managed.
There was a problem hiding this comment.
I agree with @hongquan. It's best to delete this file to ensure no sensitive information is accidentally exposed. If the value is needed, consider using environment variables or a secure vault to manage sensitive data. Let me know if you need any help with securely managing secrets!
There was a problem hiding this comment.
Great job, @Sak1012! If you need any further assistance with securely managing sensitive information or have any other questions, feel free to ask. Keep up the good work! 👍
Sak1012
force-pushed
the
badge-integration
branch
from
3988864
to
998dc45
Compare
Sak1012
force-pushed
the
badge-integration
branch
from
998dc45
to
4f382e0
Compare
hs
Outdated
| @@ -0,0 +1 @@ | |||
| sak:3670 | |||
src/components/LoginForm.vue
Outdated
|
|
||
| // router | ||
| const router = useRouter() | ||
|
|
||
| async function submitLogin() { | ||
| console.log(server.value) | ||
| if (server.value === '' || server.value === 'Select a Server') { |
There was a problem hiding this comment.
Please define constants to compare server.value with.
| <StandardButton | ||
| :type="'button'" | ||
| :text="'Register-Device'" | ||
| :disabled="false" | ||
| class="btn-primary mt-6 w-full justify-center" | ||
| @click="registerDevice" | ||
| > | ||
| </StandardButton> |
There was a problem hiding this comment.
If you are passing a static string (Register-Device) to text prop, no point to use bind syntax and JS expression.
Just pass as normal HTML attribute like this:
<StandardButton text='Register-Device' ... />
src/stores/processDevice.js
Outdated
| if (url.includes('localhost')) { | ||
| url = `${url}:8000` // Add your desired port number here |
There was a problem hiding this comment.
Please do as sourcery-ai suggested.
This also has minor fixes and removal of some `console.log()` statements and added env variable for local host testing
|
In this PR, every where you pass static string to Vue component, please use normal HTML instead of |
Sak1012
force-pushed
the
badge-integration
branch
from
ed47b8c
to
b505113
Compare
Sak1012
changed the title
This pull request contains the initial commit addressing Issue #190 (Integrate Badges Plugin with Check-in App for QR Code Scanning) in the eventyay-tickets repository.
Overview:
In this PR, a "Register Device" button has been added to the home page. This button redirects organizers to a QR code scan interface, allowing them to register their devices similarly to the functionality of pretix-scan.
Screenshots:
Home page with "Register Device" button:
QR Code Scan Interface:
QR Code Data Format:
The "Connect to Device" QR code in eventyay-tickets provides JSON data in the following format:
{ "handshake_version": 1, "url": "http://localhost", "token": "cgakvwle23d2qxwa" }Progress Checklist
panel/select-stationonce the device is connected.This feature enhances the integration between the badges plugin and the check-in app, allowing event organizers to seamlessly register and manage devices using QR codes. The implementation is inspired by the functionality of pretix-scan.
Summary by Sourcery
This pull request introduces a new feature allowing event organizers to register devices via a QR code scan interface. A 'Register Device' button has been added to the home page, leading to a new route and component for device registration. Additionally, the login form has been enhanced with a server selection dropdown and corresponding error handling.