Approach for QR integration on the attendance UI #1494
Conversation
| wp_enqueue_script( 'camptix-attendance-ui', plugins_url( '/assets/attendance-ui.js', __FILE__ ), array( 'backbone', 'jquery', 'wp-util', 'jquery-fastbutton', 'html5-qr-scanner' ), filemtime( __DIR__ . '/assets/attendance-ui.js' ) ); | ||
| wp_enqueue_style( 'camptix-attendance-ui', plugins_url( '/assets/attendance-ui.css', __FILE__ ), array( 'dashicons' ), filemtime( __DIR__ . '/assets/attendance-ui.css' ) ); |
There was a problem hiding this comment.
This is missing the HTML5 QR Code scripts, and it also duplicates the loading of the same files.
| wp_enqueue_script( 'camptix-attendance-ui', plugins_url( '/assets/attendance-ui.js', __FILE__ ), array( 'backbone', 'jquery', 'wp-util', 'jquery-fastbutton', 'html5-qr-scanner' ), filemtime( __DIR__ . '/assets/attendance-ui.js' ) ); | |
| wp_enqueue_style( 'camptix-attendance-ui', plugins_url( '/assets/attendance-ui.css', __FILE__ ), array( 'dashicons' ), filemtime( __DIR__ . '/assets/attendance-ui.css' ) ); | |
| wp_enqueue_script( 'html5-qr-scanner', plugins_url( '/assets/html5-qrcode.min.js', __FILE__ ), array( 'jquery' ) ); | |
| wp_enqueue_script( 'camptix-attendance-ui', plugins_url( '/assets/attendance-ui.js', __FILE__ ), array( 'backbone', 'jquery', 'wp-util', 'jquery-fastbutton', 'html5-qr-scanner' ) ); | |
| wp_enqueue_style( 'camptix-attendance-ui', plugins_url( '/assets/attendance-ui.css', __FILE__ ), array( 'dashicons' ) ); |
There was a problem hiding this comment.
The lib doesn't require jquery, using wp_register_script instead
| } | ||
| .qr-scanner-active #attendees-list-wrapper .qr-scanner { | ||
| display: block; | ||
| } |
There was a problem hiding this comment.
You've only added the new styles to the compiled .css file, but you should have added them to the .scss file instead and compile the .css file again.
| alert: function () { | ||
| alert('asd'); | ||
| }, |
There was a problem hiding this comment.
This should probably be removed.
| this.$header = this.$el.find('header'); | ||
| this.$qrscanner = this.$el.find('.qr-scanner'); | ||
| this.$menu = this.$header.find('.menu'); |
There was a problem hiding this comment.
The property this.$qrscanner is never used, so the changes in these lines can be reverted.
| ) { | ||
| } |
There was a problem hiding this comment.
Nothing is happening in the body of this condition. Is the this.qr(); call meant to be in the body?
There was a problem hiding this comment.
exactly, probably for testing in the wrong spot. This initialize the qr code if the settings is enabled
| this.$el.toggleClass('qr-scanner-active'); | ||
| this.$menu.removeClass('dropdown'); | ||
|
|
||
| var that = this; |
There was a problem hiding this comment.
The var keyword should be replaced with const (or let).
|
|
||
| return false; | ||
| }, | ||
|
|
There was a problem hiding this comment.
Apply WordPress coding styles on this hunk (and the rest of your changes in the file).
There was a problem hiding this comment.
need more help on setting this up in my editor (Cursor/VsCode). I always break things in trying to do so 😬
| let lastScan; | ||
| let toggleView; | ||
|
|
||
| function onScanSuccess(decodedText, decodedResult) { |
There was a problem hiding this comment.
The parameter decodedResult is never used. Can it be removed?
| ) | ||
| .fail(function (res) { | ||
| console.log('fail', res); | ||
| }) |
There was a problem hiding this comment.
Write some more helpful debug output or better remove it completely, when the PR is finished.
| }) | ||
| .always(() => { | ||
| setTimeout(() => { | ||
| //html5QrcodeScanner.resume(); |
There was a problem hiding this comment.
I'll keep this for now, it's there for a reason. Will be removed with the PR
| html5QrcodeScanner.pause(); | ||
| } | ||
|
|
||
| let c = document.getElementById('qr-reader').getBoundingClientRect(); |
There was a problem hiding this comment.
Maybe use a more verbose name for the variable.
| width: 8em; | ||
| float: right; | ||
| } | ||
| } |
There was a problem hiding this comment.
While there were changes in the .css file only before, these changes are only in the .scss file. It's not clear which library was used to compile the .css file, but I'd assume some older Sass version with the default settings.
There was a problem hiding this comment.
not sure where this is comming from, I'll keep it for now
There was a problem hiding this comment.
This file can probably be removed, since it's from the other QR code branch.
There was a problem hiding this comment.
This file can probably be removed, since it's from the other QR code branch.
There was a problem hiding this comment.
Maybe also include to non-minified file as well? Or at least comment somewhere, which library was not used.
There was a problem hiding this comment.
Not sure how it's built, that's why I left it in the first place
| @@ -76,6 +77,7 @@ | |||
| <a href="#" class="search">Search</a> | |||
| <a href="#" class="filter">Sort & Filter</a> | |||
| <a href="#" class="refresh">Refresh</a> | |||
| <a href="#" class="qr">QR Scanner</a> | |||
There was a problem hiding this comment.
Add a condition to only show this when the QR code scanner is enabled (or use a CSS class, switched by JS, to hide it - but better not render it at all).
There was a problem hiding this comment.
I think it was supposed to be always there (the menu entry): The settings should just show the camera field by default, so you don't have to open it manually every time.
Since it's in the menu entry, I would keep it as it is and just show the camera, depending on the setting.
There was a problem hiding this comment.
Ah, so if the setting is set to "On", the camera would always show, and with "Off", only when accessed thought the menu?
There was a problem hiding this comment.
Yes, so if you reload the page you don't have to click into the menu. We can change this behavior of course
| @@ -88,13 +90,15 @@ | |||
| <span>Loading...</span> | |||
| </li> | |||
| </ul> | |||
| <div class="qr-scanner"><div id="qr-reader"></div></div> | |||
There was a problem hiding this comment.
Are these two DIVs a requirement from the JS library? Wouldn't one be sufficient enough?
There was a problem hiding this comment.
Yes, I think it was because the outer was for styling and the inner just contained the canvas. I'll check that though.
| @@ -80,6 +82,11 @@ public function ajax_callback() { | |||
| if ( empty( $_REQUEST['camptix_secret'] ) || $_REQUEST['camptix_secret'] != $this->secret ) | |||
| return; | |||
|
|
|||
| // get qr code response if requested | |||
| if ( isset( $_GET['qrcode'] ) ) { | |||
There was a problem hiding this comment.
Use $_REQUEST instead of $_GET, as with the other request parameters.
| $qrcode = sanitize_text_field( $_GET['qrcode'] ); | ||
|
|
||
| if ( ! preg_match( '/^[0-9a-f]{16}$/', $qrcode ) ) { | ||
| //return wp_send_json_error( 'invalid qrcode format' ); |
There was a problem hiding this comment.
This should do something.
| */ | ||
| public function _ajax_qrcode() { | ||
|
|
||
| $qrcode = sanitize_text_field( $_GET['qrcode'] ); |
There was a problem hiding this comment.
That might not be needed, if we return early, if the format does not match.
There was a problem hiding this comment.
yes, but check the value in the _ajax_qrcode method.
| global $wpdb; | ||
|
|
||
| // get the attende ID by the qrcode | ||
| // this is maybe resource intensive for larger events allthough we don't had any WordCamp > 3000 tickets so I guess we're fine |
There was a problem hiding this comment.
This comment should be moved from code to the PR.
| } | ||
| // if ( ! $attendee || 'tix_attendee' != $attendee->post_type || 'publish' != $attendee->post_status ) { | ||
| // return wp_send_json_error( 'user not found' ); | ||
| // } |
| $where = $wpdb->prepare( | ||
| " | ||
| ( | ||
| tix_first_name.meta_value LIKE '%%%s%%' OR | ||
| tix_last_name.meta_value LIKE '%%%s%%' OR | ||
| CONCAT( tix_first_name.meta_value, ' ', tix_last_name.meta_value ) LIKE '%%%s%%' | ||
| ) | ||
| ", $search, $search, $search ); | ||
|
|
||
| return $clauses; | ||
| } ); | ||
| ", | ||
| $search, | ||
| $search, | ||
| $search | ||
| ); |
There was a problem hiding this comment.
Indent these lines correctly.
| $clauses['join'] .= " | ||
| INNER JOIN $wpdb->postmeta tix_access_token ON ( ID = tix_access_token.post_id AND tix_access_token.meta_key = 'tix_access_token' ) | ||
| "; |
There was a problem hiding this comment.
Indent these lines correctly.
| $where = '(' . | ||
| $where . | ||
| ' OR (' . | ||
| $wpdb->prepare( "tix_access_token.meta_value LIKE '%%%s'", $search ) . | ||
| ')' . | ||
| ')'; | ||
| } | ||
|
|
||
| $clauses['where'] .= ' AND ' . $where; |
There was a problem hiding this comment.
Maybe put the previous clauses into two variables and "join" them differently, then overwriting $where multiple times.
There was a problem hiding this comment.
we could do an array an join them later? Skip this for now
| 'field_yesno', | ||
| 'general', | ||
| esc_html__( 'Enable QR code scanning?', 'wordcamporg' ) | ||
| ); |
There was a problem hiding this comment.
As much as I prefer this wrapping in multiple lines, maybe write it in one line like the surrounding lines.
| </div> | ||
| </script> | ||
|
|
||
| <script id="tmpl-attendee-search" type="text/template"> | ||
| <a href="#" class="close dashicons dashicons-no"></a> | ||
| <div class="wrapper"> | ||
| <input type="text" autocomplete="off" placeholder="Search" /> | ||
| <div class="previews"></div> |
There was a problem hiding this comment.
not sure why this is here. It's also referenced in the scss above. I'll keep it for now
|
I like to write this down somewhere. The QR code is based on the last 16 digits of the MD5 value of the access_token of the attendee. This is of course debatable but I will use this for now. |
See #613