Adds drawn signature to asset acceptance #2846

snipe · 2016-10-29T20:44:23Z

No description provided.

This is still a little broken - the history is displaying “Deleted user”, since there is no item type listed. Saving the item_type as App\Models\User tries to update accepted on the users table, which obviously doesn’t exist.

It shoudl fail if you’re not authorized since you’re not logged in, but better safe than sorry

dmeltzer

Some thoughts/opinions. Most of these are more me pondering what we do about actionlogs in the future than specifics here-

One thought I had when looking through this is whether declined acceptance should require a signature, or just accepting it. Presently it looks like it's both, but not sure that is what everyone will expect?

dmeltzer · 2016-10-31T01:04:17Z

app/Http/Controllers/ActionlogController.php

+ {
+
+ $file = config('app.private_uploads') . '/signatures/' . $filename;
+ $filetype = Helper::checkUploadIsImage($file);


Maybe we want to rename this helper method? I don't expect a "checkXisY" method to return anything but a boolean.

dmeltzer · 2016-10-31T01:09:33Z

app/Http/Controllers/ReportsController.php

@@ -378,6 +380,9 @@ public function getActivityReportDataTable()
 } else {
 $activity_target = '';
 }
+ } elseif (($activity->action_type=='accepted') || ($activity->action_type=='declined')) {
+ $activity_target = '<a href="' . route('view/user', $activity->item->assigneduser->id) . '">' . e($activity->item->assigneduser->fullName()) . '</a>';
+


Separate from this... but I'm thinking maybe we want to go towards a Helper::generateHtmLinkFromItem($item) that does something like

return "a href=" . route("view/${metaprogramming_classname_somethigorother_from_item}", $item->id) . ">" . $item->displayableName . "</a>"

because we seem to do this so often... not sure if this is the best way though.. thoughts?

dmeltzer · 2016-10-31T01:13:18Z

app/Http/Controllers/ViewAssetsController.php

+ $encoded_image = explode(",", $data_uri);
+ $decoded_image = base64_decode($encoded_image[1]);
+ file_put_contents($path."/".$sig_filename, $decoded_image);
+ }


Another place that has a lot of specifics about file writing.. . should we abstract these bits to a method or just wait for the 5.3 switch where laravel does it for us?

Actually--don't we have access to a Symfony uploadedFile instance here? That would let us use move() and guessFileExtension() and things like that.

dmeltzer · 2016-10-31T01:14:09Z

app/Http/Controllers/ViewAssetsController.php

 $logaction->note = e(Input::get('note'));
- $logaction->user_id = $user->id;


Was this an intentional removal of the user ID? Hard to tell from this context, just want to make sure.

user_id has typically been a strictly admin id (it's how to populate "Admin" everywhere). There is no admin involved in the acceptance of a asset. It sort of doesn't make any sense to make them a target either, but everywhere else, user_id = admin actor

dmeltzer · 2016-10-31T01:17:09Z

app/Models/Actionlog.php

@@ -91,15 +90,14 @@ public function parentlog()
 /**
 * Check if the file exists, and if it does, force a download
 **/
- public function get_src($type = 'assets')
+ public function get_src($type = 'assets', $fieldname = 'filename')


This seemed like a strange place for this method to exist to me when I was polymorphicating everything, but I was afraid to touch it... also unclear exactly what the method does when looking at it quickly. Does associating files with a log in general make sense? (The signatures do, wondering about the asset files)--Not really related to your sig, just a ponder I had when staring at all this code a few weeks ago.

I think we have them in the log action table strictly for ease of use when displaying history stuff. Otherwise we'll end up pulling in pivot tables for uploads, etc.

dmeltzer · 2016-10-31T01:17:41Z

database/migrations/2016_10_29_082408_add_signature_to_acceptance.php

+ {
+ Schema::table('settings', function ($table) {
+ $table->dropColumn('require_accept_signature');
+ $table->dropColumn('accept_signature');


This line shouldn't exist, right? It belongs to the action_logs table.

Yep, you're right - good catch, thanks!

dmeltzer · 2016-10-31T01:20:08Z

resources/views/account/accept-asset.blade.php

+ </div>
+ @endif
+
+ @if (\App\Models\Setting::getSettings()->require_accept_signature=='1')


Wonder if we want to look at injecting settings globally, could be nice to always have access to a $settings variables in views to clean this up.

Easy enough to do via middleware, but I think we'd still need something like $request->settings->blah, which doesn't seem shorter. We cache the value of the settings query at least, so we're not running it every time we need a setting.

Looks like we can use the hard to find view()->share() method in an app service provider to share a value with all views... https://laravel.com/docs/5.3/views#passing-data-to-views

I could see this getting confusing if we had to many of these, wonder if making a global settings() or snipeSettings() variable would be more clear.

snipe · 2016-10-31T05:48:55Z

These are all good suggestions, but most are not directly related to this PR :) Thanks for taking a look.

* Adds digital signature to asset acceptance This is still a little broken - the history is displaying “Deleted user”, since there is no item type listed. Saving the item_type as App\Models\User tries to update accepted on the users table, which obviously doesn’t exist. * Use asset facade for folks in subdirs * Possible fix for weird accepted/declined display * Display signature in modal popup if sigs are required * Wrap that display file in auth middleware, just to be sure. It shoudl fail if you’re not authorized since you’re not logged in, but better safe than sorry * Fixed header section of layout * Removed extra drop from migration rollback

snipe added 5 commits October 29, 2016 11:39

Adds digital signature to asset acceptance

ff8c7be

This is still a little broken - the history is displaying “Deleted user”, since there is no item type listed. Saving the item_type as App\Models\User tries to update accepted on the users table, which obviously doesn’t exist.

Use asset facade for folks in subdirs

dd0535c

Possible fix for weird accepted/declined display

bed1407

Display signature in modal popup if sigs are required

8cb8c63

Wrap that display file in auth middleware, just to be sure.

8152340

It shoudl fail if you’re not authorized since you’re not logged in, but better safe than sorry

snipe added the in progress label Oct 29, 2016

Fixed header section of layout

057f4ff

dmeltzer reviewed Oct 31, 2016

View reviewed changes

Removed extra drop from migration rollback

3b33e4d

snipe merged commit a914dac into develop Nov 1, 2016

snipe removed the in progress label Nov 1, 2016

snipe deleted the features/drawn_signature branch November 17, 2016 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds drawn signature to asset acceptance #2846

Adds drawn signature to asset acceptance #2846

snipe commented Oct 29, 2016

dmeltzer left a comment

dmeltzer Oct 31, 2016

dmeltzer Oct 31, 2016

dmeltzer Oct 31, 2016

dmeltzer Oct 31, 2016

dmeltzer Oct 31, 2016

snipe Oct 31, 2016

dmeltzer Oct 31, 2016

snipe Oct 31, 2016

dmeltzer Oct 31, 2016 •

edited

Loading

snipe Oct 31, 2016

dmeltzer Oct 31, 2016

snipe Oct 31, 2016

dmeltzer Oct 31, 2016

snipe commented Oct 31, 2016

		$logaction->note = e(Input::get('note'));
		$logaction->user_id = $user->id;

Adds drawn signature to asset acceptance #2846

Adds drawn signature to asset acceptance #2846

Conversation

snipe commented Oct 29, 2016

dmeltzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmeltzer Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snipe commented Oct 31, 2016

dmeltzer Oct 31, 2016 •

edited

Loading