Fixed a few field entries #116

web541 · 2018-01-03T10:31:53Z

Some more small issues I've found.

These would not save otherwise.

Wouldn’t save when importing from phpvms classic.

Change arguments to ask in terminal Fixed table_prefix names in Importer.php Added the ability to import users from phpvms classic (tested on simpilot’s 5.5.x) and when importing, it will then reset the user’s password to a temporary hash and then email it to the user to then change when they first log in.

web541 · 2018-01-03T10:55:30Z

And also, could you check this commit out I added the functionality to import users from simpilot's 5.5.x classic version to this version.

The way it works is that it takes all the pilots from the classic DB, adds them into the new DB, generates their API key & temporary password hash and finally emails them this info to log in. When they first log in, they should be on their edit profile page to change their password. Saves them going through the effort of requesting a password change first up, although either way they still have to check their emails.
Didn't want to push it through just yet in case you had other ideas yourself, in that case I'll just discard it, no issues.

EDIT: It seems it actually made it into this one (thanks Github), you can remove it if you wish.

web541 · 2018-01-03T11:03:03Z

app/Http/Controllers/Auth/LoginController.php

@@ -57,6 +57,12 @@ protected function sendLoginResponse(Request $request)
        $request->session()->regenerate();
        $this->clearLoginAttempts($request);

+        # Check if it's their first login to reset their password
+        if($user->created_at == $user->updated_at) {


This will have to end up being changed anyway, just thought that now anyone who has registered after the import will have to change their password which isn't ideal.

Leave this bit out for now, lemme think about this one. It might make more sense to add a boolean column of reset_password, and check against that. That way we can use it in other cases, like a lockout. There's also the reset password function that uses a token. Maybe checking if there's a token filled, I have to look at the forgot password code.

nabeelio · 2018-01-03T11:47:37Z

app/Console/Commands/Importer.php

@@ -7,7 +7,7 @@

 class Importer extends BaseCommand
 {
-    protected $signature = 'phpvms:importer {db_host} {db_name} {db_user} {db_pass?}';


Not sure where my comment went, change this one back for now, I'll probably change this to use a config file, since I think there will be more settings to add. For testing, though, it's a pain with the ask() every time

nabeelio · 2018-01-03T11:48:18Z

app/Console/Services/Importer.php

@@ -68,6 +72,7 @@ public function __construct($db_creds)
            'name' => '',
            'user' => '',
            'pass' => '',
+            'table_prefix' => ''


set to 'table_prefix' => 'phpvms_' as the default

nabeelio · 2018-01-03T11:48:42Z

app/Console/Services/Importer.php

@@ -163,7 +172,7 @@ protected function saveModel($model)
            $model->save();
            return true;
        } catch (QueryException $e) {
-            #$this->error($e->getMessage());
+            #return $this->error($e->getMessage());


Leave this in for now, still debugging and working on this

nabeelio · 2018-01-03T11:50:41Z

app/Console/Services/Importer.php

        $offset = 0;
-        $total_rows = $this->getTotalRows($table);
+        $total_rows = intval($this->getTotalRows($table));


I wouldn't put this here. In the getTotalRows() method, do the return (int) $rows I believe it would be

nabeelio · 2018-01-03T11:52:12Z

app/Console/Services/Importer.php


+            $name = $row->firstname.' '.$row->lastname;
+            $airlineid = Airline::where('icao', $row->code)->first()->id;


We don't need to do lookups here. When populating the airport and ranks, I filled out $this->airports and $this->ranks (see those import functions). So we can do a lookup in those arrays, vs having to do a DB call every time.

$airline_id = $this->airlines[$row->code) $rank_id = $this->ranks[$row->rank]

nabeelio · 2018-01-03T11:53:54Z

app/Console/Services/Importer.php

+        foreach($allusers as $user)
+        {
+            # Generate New User Password
+            # TODO: Increase Security?


Nah, this is fine

nabeelio · 2018-01-03T11:54:44Z

app/Console/Services/Importer.php

+            $apikey = Utils::generateApiKey();
+
+            # Update Info in DB
+            $user->password = bcrypt($newpw);


If you look in the registration code, there's a Hash::make() or something like that which will salt. Just to make sure that the password check works properly. That might use bcrypt under the hood but let's still call that one to make sure we're 100% compat

nabeelio · 2018-01-03T11:55:59Z

app/Console/Services/Importer.php

+            $user->save();
+
+            # Send E-Mail to User with new login details
+            $email = new \App\Mail\NewLoginDetails($user, $newpw, 'New Login Details | '.config('app.name'));


Don't email for now. We'll have to figure this bit out. Might be a separate option to do it, and we'll do it at the end, in a batched manner, after the import has completed. Otherwise it slows the user import down considerably

nabeelio · 2018-01-03T11:58:09Z

app/Models/User.php

        'api_key',
        'home_airport_id',
        'curr_airport_id',
        'last_pirep_id',
-        'rank_id',
+        'flights',


Good catch on these!

nabeelio · 2018-01-03T11:59:28Z

app/Console/Services/Importer.php

+     */
+    protected function getUserState($state)
+    {
+        // Decide which state they will be in accordance with v7


This looks good... one small change, I like to be explicit. Maybe add an array here,

$phpvms_classic_states = [ 'ACTIVE' => 0, ... ];

And then check against those.

nabeelio · 2018-01-03T12:00:06Z

app/Console/Services/Importer.php

+                'rank_id' => $rankid,
+                'home_airport_id' => $row->hub,
+                'curr_airport_id' => $row->hub,
+                'flights' => intval($row->totalflights),


It's more efficient to use (int) $row->totalflights

nabeelio · 2018-01-03T12:01:32Z

app/Console/Services/Importer.php

+                'home_airport_id' => $row->hub,
+                'curr_airport_id' => $row->hub,
+                'flights' => intval($row->totalflights),
+                'flight_time' => intval($row->totalhours),


This actually needs to be converted to minutes. There's a Utils::hoursToMinutes()

nabeelio · 2018-01-03T12:03:05Z

Hey, this looks mostly good, just a bunch of changes. Thanks a ton, man! I'm glad for the help. Also, if you want to split the PRs, you need to create new branches in your local repo, push those, and then that will let you create PRs against master

nabeelio · 2018-01-03T12:07:29Z

Another thing that needs to be done (if you want to), in the user import, we need to get all of the groups, specifically, making sure that the users in the Administrators group are added to the admin role. And all of the users need to be added to the users role. See

https://github.com/nabeelio/phpvms/blob/master/app/Services/UserService.php#L35

I'd make that a separate PR, since that'll need some changing to the save code and stuff, and making sure the saveModel() works by-reference (I believe it does by default, unless IntelliJ is lying to me)

nabeelio · 2018-01-03T12:13:45Z

app/Console/Services/Importer.php

+            $newpw = substr(md5(date('mdYhs')), 0, 10);
+
+            # Generate API Key
+            $apikey = Utils::generateApiKey();


I don't mean to nitpick :) But can you change these all to $api_key, etc, just keep the underscores in there. I just want the variables and everything to be uniform across the code

web541 · 2018-01-03T20:24:44Z

Yep, I'll make those changes, the way you've made v7 is different to the way I would've, but it's a fun learning experience.
+1 on the branch in the fork, I didn't think of it that way.

nabeelio · 2018-01-03T20:29:34Z

@web541 cool, thanks! If you hop on Discord, I'd like to hear about what you would've done differently, there might be some ideas in there as well

web541 · 2018-01-03T21:20:09Z

app/Console/Services/Importer.php

+
+            $name = $row->firstname.' '.$row->lastname;
+            $airline_id = $this->airlines[$row->code];
+            $rank_id = $this->ranks[$row->rank];


This gets the rank before it's imported, so the id remains as rankid in the old DB. Any ideas on how to change this to get the imported rank in the new DB?

Ah good point! You can put it back to the DB lookup. I'll think of something more efficient tonight

D'oh, I remember what I was meaning to do. I was gonna set the value to the new ID from the insert. I'll fix it later.

All good, this might be the same with airlines as well (only tested with the one that was already there, so not sure).

Yeah, I'll fix it. Good to merge?

I think so, I'll add the admin permissions thingo in another branch after so it doesn't stuff the current stuff up.

And also for that, importUsers function or separate function?

Umm I gotta thing about that. I might have to do a bit of refactoring for the ID mapping stuff. Probably makes sense to go into the method that's resetting the password/api key, etc. But that too will likely be once I refactor the mapping thing in a couple of hours

np, I'll leave it for now.

web541 added 4 commits January 3, 2018 21:24

Stopped inheritance errors popping up

d7b3b8e

Added fillable fields

2705609

These would not save otherwise.

Added country fillable field

3cd5dc0

Wouldn’t save when importing from phpvms classic.

web541 commented Jan 3, 2018

View reviewed changes

nabeelio reviewed Jan 3, 2018

View reviewed changes

app/Models/User.php

'api_key',

'home_airport_id',

'curr_airport_id',

'last_pirep_id',

'rank_id',

'flights',

Copy link

Owner

nabeelio Jan 3, 2018

Choose a reason for hiding this comment

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

Good catch on these!

nabeelio reviewed Jan 3, 2018

View reviewed changes

Changes to ImporterService

a15dbaf

web541 commented Jan 3, 2018

View reviewed changes

Merge branch 'master' into master

bcb2ac3

nabeelio merged commit 08df20d into nabeelio:master Jan 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed a few field entries #116

Fixed a few field entries #116

web541 commented Jan 3, 2018

web541 commented Jan 3, 2018 •

edited

Loading

web541 Jan 3, 2018

nabeelio Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018

nabeelio Jan 3, 2018

nabeelio commented Jan 3, 2018 •

edited

Loading

nabeelio commented Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018

web541 commented Jan 3, 2018

nabeelio commented Jan 3, 2018

web541 Jan 3, 2018

nabeelio Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018

web541 Jan 3, 2018

nabeelio Jan 3, 2018

web541 Jan 3, 2018

web541 Jan 3, 2018

nabeelio Jan 3, 2018

web541 Jan 3, 2018


		$name = $row->firstname.' '.$row->lastname;
		$airlineid = Airline::where('icao', $row->code)->first()->id;

Fixed a few field entries #116

Fixed a few field entries #116

Conversation

web541 commented Jan 3, 2018

web541 commented Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

nabeelio Jan 3, 2018 • 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

nabeelio Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabeelio Jan 3, 2018 • 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

nabeelio commented Jan 3, 2018 • edited Loading

nabeelio commented Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

web541 commented Jan 3, 2018

nabeelio commented Jan 3, 2018

Choose a reason for hiding this comment

nabeelio Jan 3, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

web541 commented Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018 •

edited

Loading

nabeelio commented Jan 3, 2018 •

edited

Loading

nabeelio commented Jan 3, 2018 •

edited

Loading

nabeelio Jan 3, 2018 •

edited

Loading