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

Issue Fix: Issue 165 - isAdmin state is moved to zustand store and associated functionality is changed #210

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

Adit-K06
Copy link
Contributor

Summary: What does this PR do?

Which issue(s) this PR fixes

Changes Made

Describe the changes you've made in this PR:

  • Feature implementation
  • [✓] Bug fix
  • Documentation update
  • Code refactoring
  • Configuration changes
  • Other (please specify)

Type of Change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New feature (non-breaking change which adds functionality)
  • [✓ ] Bug fix (non-breaking change which fixes an issue)
  • Documentation update
  • Performance improvement
  • Code cleanup or refactor

How Has This Been Tested?

Describe the tests you ran:

  • Unit tests
  • Integration tests
  • [✓ ] Manual tests
  • Other (please specify)

Please describe the test cases and expected behavior:

  1. When the Admin is logged in to the website the respective webpages must have the feature to change the components of the webpage. And if any other user logs in or the admin logs out then, the features to change the components of the webpage must be hidden.
  2. For example : If the admin is logged in, the events page shows the features of adding an additional event or removing / editing an existing an existing even, whereas if the admin logs out or another user logs in these features must be hidden.

Screenshots (if applicable)

Dependencies

Documentation

  • I have updated the documentation accordingly
  • [✓ ] Documentation update is not required

Comments:

Reviewer Notes

.infisical.json Outdated Show resolved Hide resolved
Copy link

@akashsinghvance akashsinghvance left a comment

Choose a reason for hiding this comment

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

Kindly make these changes and attach a screen recording of the admin functinality working as intended. Collect admin creds from Alfiya

lib/zustand/store.ts Outdated Show resolved Hide resolved
@Adit-K06 Adit-K06 marked this pull request as draft February 1, 2025 12:55
@Adit-K06
Copy link
Contributor Author

Adit-K06 commented Feb 1, 2025

Removed all the unwanted comments from the code base and also attaching a video showcasing the admin functionality.

PB-isAdmin-Functionality.1.mp4

@Adit-K06 Adit-K06 marked this pull request as ready for review February 1, 2025 13:00
Copy link
Member

@SkySingh04 SkySingh04 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@SkySingh04 SkySingh04 left a comment

Choose a reason for hiding this comment

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

Please resolve the above comments, rest LGTM

setIsAdmin(isAdmin);
setIsAdminLoggedIn(true);
setAdmin(isAdmin);
setAdmin(true);
Copy link
Member

Choose a reason for hiding this comment

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

redundant code, setting it to isAdmin and then again to true

} catch (error) {
console.error("Error checking admin status:", error);
setIsAdmin(false);
setIsAdminLoggedIn(false);
setAdmin(false);
Copy link
Member

Choose a reason for hiding this comment

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

redundant code, setting it to isAdmin and then again to false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}
};

const unsubscribe = onAuthStateChanged(auth, (user) => {
if (user) {
checkAdmin(user.uid);
} else {
setIsAdmin(false);
setIsAdminLoggedIn(false);
setAdmin(false);
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -1,5 +1,9 @@
"use client";


import { useStore } from "@/lib/zustand/store";

Copy link
Member

Choose a reason for hiding this comment

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

nit : newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


reset();
}
=======
Copy link
Member

Choose a reason for hiding this comment

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

why is the ==== here? maybe the merge wasnt done properly

@areebahmeddd
Copy link
Contributor

Crazy work! LGTM

Copy link
Contributor

@alfiyafatima09 alfiyafatima09 left a comment

Choose a reason for hiding this comment

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

LGTM !

@SkySingh04 SkySingh04 merged commit b7edd44 into pbdsce:prod Feb 4, 2025
1 check passed
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.

6 participants