Complete Login and Signup modules in MVT Framework by fatimarafiqui · Pull Request #232 · systers/FirstAide-web

fatimarafiqui · 2017-06-07T09:24:18Z

The following has been implemented and achieved in this section [ Trello Task ]

User and Authentication class modules and methods
Validation of user data for login and signup
Added npm_module directory and setup of gulp for minification of stylesheets
AJAX handlers for proper authentication in login/signup
UI Enhancement of the complete login/signup and incorporating responsiveness.
Sanitisation of GET and POST data.
Rewritten myqli based on OOPs concept.
Updation of HOST to localhost root
Updating gitignore

Testing Responsiveness:

…le to User module

prathmeshranaut · 2017-06-07T09:27:06Z

+
+	$_GET   = filter_input_array(INPUT_GET, FILTER_SANITIZE_STRING);
+	$_POST  = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING);
+?>


Please remove the closing PHP tag.

And a php file should end with a single blank line.

Can you please do the above two changes for all the php files.

This was already done during the day. I just have to push the code. Thanks @prathmeshranaut

prathmeshranaut · 2017-06-07T10:21:45Z

 <?php
 	class Authentication {
-		private $user;
+		public $user;


You should make this private and use getters to get this variable in any other file.
Think of this from a software design perspective. You would never want any other script to modify the user data in the authentication file.

@prathmeshranaut Agreed, I will do this change. Thanks for pointing it out.

prathmeshranaut · 2017-06-07T10:30:27Z

-				return false;
+		public function __construct($email, $password) {
+			if (!$email || !$password) {
+				return NULL;


Instead of returning null in a constructor, throw an exception. This is sort of a standard behavior.

I read it somewhere, it is advised to throw an exception when it is truly an error, here what I am doing is making sure that the object doesn't exist in case the email and password are not provided. Check this @prathmeshranaut

Check the standard coding guidelines once.

prathmeshranaut · 2017-06-07T10:35:45Z

+						) VALUES (?, ?, ?, ?, ?)"
+					);
+					$stmt->bind_param (
+						'sssss',


Whats this?

Here I am using prepared statement which is designed for sending safe query to db server, this can be done by escaping user input which is not part of the real query, and also checking the query. This is what I have done.

MehaKaushik · 2017-06-07T15:47:55Z

+
+	$_GET   = filter_input_array(INPUT_GET, FILTER_SANITIZE_STRING);
+	$_POST  = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING);
+?>


And a php file should end with a single blank line.

MehaKaushik · 2017-06-07T15:53:35Z

@@ -1,10 +1,14 @@
 <?php
-	// mysqli connection
-	//require_once('includes/settings.php');


Do not include dead codes in comments. Simply remove the LOCs which are not required.

Are you talking about the commented code in line 3? Because I do not see any dead code. @MehaKaushik

commented code in line 3

@MehaKaushik You have commented on the wrong section of the file. That's the old code, which I have already removed.

Hence, taking no action.

Hey, sorry my bad. Thanks for correcting me.

MehaKaushik · 2017-06-07T16:06:31Z

+			} else {
+				return NULL;
+			}
+		}


I do not think returning NULL is a good idea. Can we change it to something else?

#232 (comment) Explained it here. This is the standard way of making sure that object does not exist. @MehaKaushik

MehaKaushik · 2017-06-07T16:12:48Z

+					$stmt->close();
+					return $affected;
+				} else {
+					return false;


Multiple return statements are tried to be avoided. But Sometimes they cannot be.
Here, the second return can be removed. Instead of going into "else", control would go out and meet the other "return" statement. (Logically, can remove one of the three, returns)

@MehaKaushik I will fix this.

MehaKaushik · 2017-06-07T16:43:14Z

 <?php
+	$APPLICATION_DIR = $APPLICATION_DIR ?? str_replace('includes', '', dirname(__FILE__));
 	spl_autoload_register(function ($class_name) {
 		global $APPLICATION_DIR;


According to PSR2 standards, opening braces of a method MUST go onto the next line. This should hold for functions also, can you please check and change accordingly.

@MehaKaushik Agree, will make the change.

@MehaKaushik Checked with the documentation. This is a function and therefore braces should remain as it is. I have incorporated the changes for all methods and classes, and verified it with the php code sniffer.

fatimarafiqui · 2017-06-07T19:03:29Z

@MehaKaushik @prathmeshranaut Done with all the required changes.

prathmeshranaut · 2017-06-09T16:52:05Z

    "twig/twig": "~2.0",
-    "twilio/sdk": "^5.10"
+    "twilio/sdk": "^5.10",
+    "squizlabs/php_codesniffer": "*"


Could you please move this to a require-dev block.

fatimarafiqui added 14 commits June 7, 2017 13:21

set session start and sanitized the get and post data

5cbba6f

Reused mysqli using OOPs concepts

242309e

added csrf token at the bottom of all pages for session validation

202613f

Adding styles for login

58d02fd

Added user authentication class modules and methods

37ba437

set HOST to localhost root

83095d5

Added jsonify and email validation for login and sign up

1a1c7a3

UI for login and signup with firstaide logo

d6ec821

updated routing urls and added country array

d0da538

Added User module and move the relevant code from Authentication modu…

508016a

…le to User module

added country list json and js for login/signup

5e042d0

Added gulp for minification of stylesheets and updated sign up UI

b2d11e1

Added AJAX call handler for login and signup

8f062e0

Added node_modules directory to gitignore

9849a7f

prathmeshranaut reviewed Jun 7, 2017

View reviewed changes

Updated readme section for node_modules directory

1f8bda8

prathmeshranaut suggested changes Jun 7, 2017

View reviewed changes

prathmeshranaut reviewed Jun 7, 2017

View reviewed changes

fatimarafiqui mentioned this pull request Jun 7, 2017

Fixes #163: Resolved the problem of refilling the data on registration page when duplicate email validation failes #226

Closed

MehaKaushik suggested changes Jun 7, 2017

View reviewed changes

MehaKaushik reviewed Jun 7, 2017

View reviewed changes

fatimarafiqui force-pushed the wip/auth branch from 15c6748 to 76967d0 Compare June 7, 2017 18:58

fatimarafiqui force-pushed the wip/auth branch from 76967d0 to ab5feca Compare June 7, 2017 19:24

PSR-2 Standards Enforcement

694c6d7

fatimarafiqui force-pushed the wip/auth branch from ab5feca to 694c6d7 Compare June 7, 2017 20:28

prathmeshranaut reviewed Jun 9, 2017

View reviewed changes

prathmeshranaut approved these changes Jun 9, 2017

View reviewed changes

MehaKaushik approved these changes Jun 9, 2017

View reviewed changes

MehaKaushik merged commit 47cdca0 into systers:develop Jun 9, 2017

Conversation

fatimarafiqui commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatimarafiqui Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatimarafiqui Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatimarafiqui Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatimarafiqui commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

fatimarafiqui commented Jun 7, 2017 •

edited

Loading

fatimarafiqui Jun 7, 2017 •

edited

Loading

fatimarafiqui Jun 7, 2017 •

edited

Loading

fatimarafiqui Jun 7, 2017 •

edited

Loading