Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PRESS4-430, PRESS4-435 && PRESS4-430 #3

Merged
merged 103 commits into from
Feb 1, 2024
Merged

PRESS4-430, PRESS4-435 && PRESS4-430 #3

merged 103 commits into from
Feb 1, 2024

Conversation

ramyakrishnai
Copy link
Contributor

  • Handle Popup blocker scenario for FB login form

  • We need to move helper functions to get user profile details, images etc to PHP from Javascript.

  • Encrypt/Decrypt Facebook access_token

  • Moved hiive token generation logic to the wp-module-facebook module.

  • Checking access_token validity logic.

  • Notification added to notify the customer once Facebook is connected.

  • Added encryption to fb_token in facebook.

  • Moved helper functions logic to backend.

Jira Links

Copy link
Member

@wpscholar wpscholar left a comment

Choose a reason for hiding this comment

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

I left a few comments that I think we should address, but that aren't necessarily blockers.

The main things I'd like to see changed are using the wp_hash() function and the return type issue. The others are just quick typo fixes and code readability changes.

{
$hiive_token = HiiveConnection::get_auth_token();
$hash_token = get_option('nfd_fb_hash_hiive_token');
$encryoted_token = null;
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed we have a typo here. It will still work, but probably should fix.

update_option('nfd_fb_details', $FacebookData);
return array($FacebookData);
}
return 'token not found!';
Copy link
Member

Choose a reason for hiding this comment

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

This looks to have a different return type than everything else. We're returning an array on success or an array with an error property on error, except here. We should probably be returning a WP_Error object for errors. This way, we can easily check for is_wp_error().

UtilityService::storeTokenInCookie($details);
}

$encrpt = new Encryption();
Copy link
Member

Choose a reason for hiding this comment

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

Another typo here

{
$token = json_encode($token);
// setting cookie for 2 months
setcookie('fb_access_token', $token, time() + (60 * 60 * 24 * 30 * 2));
Copy link
Member

Choose a reason for hiding this comment

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

We should use the built-in constants to make this more human-readable and less prone to error

Suggested change
setcookie('fb_access_token', $token, time() + (60 * 60 * 24 * 30 * 2));
setcookie('fb_access_token', $token, time() + (MONTH_IN_SECONDS * 2));

*/
public static function deleteTokenFromCookie()
{
setcookie('fb_access_token', '', time() - (60 * 60 * 24 * 30 * 2));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
setcookie('fb_access_token', '', time() - (60 * 60 * 24 * 30 * 2));
setcookie('fb_access_token', '', time() - (MONTH_IN_SECONDS * 2));

},height=${window.innerHeight / 2 + 200},top=200,left=200`
);

const intervalId = setInterval(async function () {
Copy link
Member

Choose a reason for hiding this comment

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

@ramyakrishnai Still curious about this one.

$hash_token = get_option('nfd_fb_hash_hiive_token');
$encryoted_token = null;
if (!$hash_token) {
$encryoted_token = $hiive_token ? UtilityService::encrypt_token($hiive_token) : '';
Copy link
Member

Choose a reason for hiding this comment

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

@ramyakrishnai This isn't really a hashed token. Since we are only using this as a lookup value, we should use an irreversible hash here. We just need to run the token through the wp_hash() function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @wpscholar the setInterval, we're using to get notified when the pop up is closed.So, we can do a api call when it is closed to get the user details
Thanks!

Copy link
Member

@wpscholar wpscholar left a comment

Choose a reason for hiding this comment

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

Looks good! There are still some small, nice-to-have changes, but given we've got a big launch in the works, this resolves all the big issues and is ready for public use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants