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.

Sign up for GitHub

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

Jump to bottom

added google oauth #101

Merged
merged 8 commits into from
Jan 15, 2025
Merged

added google oauth #101

merged 8 commits into from
Jan 15, 2025

Conversation

disvid
Copy link
Collaborator

@disvid disvid commented Jan 10, 2025 •
edited
Loading

Related Issue

Closes #72

Type of Change

Put x inside the square bracket to specify what type of change your PR is:

  • New Feature
  • Bug Fix
  • Code Refactor
  • Documentation Update
  • Other (please specify):

Description of Change

added google oauth verification.

Demo

Screencast.from.2025-01-11.00-28-27.webm

@rkcoder101
Copy link
Collaborator

rkcoder101 commented Jan 13, 2025 •
edited
Loading

are able to edit the tickets after adding the oauth feature? because i am unable to do so

@disvid
Copy link
Collaborator Author

disvid commented Jan 13, 2025

@rkcoder101 i have made the changes and now the ticket is editable

@AyushDharDubey
Copy link
Collaborator

Hi @disvid, great work taking up this one, but you seriously should have discussed your approach first on the issue page. the pr lacks clarity on the implementation flow and we’re out of time to address any major fixes, we will discuss and let you know whether we can merge this one or not.

in the meantime, feel free to occupy yourself in following changes.

  1. restore SAMPLE.env.local and app/api/Tickets/[id]/route.js as they were.
  2. while adding the signout button, you have removed toogleTheme button and img icon, restore them
  3. update SAMPLE.env.local and README.md to display the new env vars setup.
  4. restore the metadata object in app/layout.jsx and remove the app/metadate.js, refactoring changes are beyond the scope of this PR.

@disvid
Copy link
Collaborator Author

disvid commented Jan 13, 2025 •
edited
Loading

Hello sir @AyushDharDubey ,
I understand your concern - but now the feature is working smoothly and I request you to consider merging this.

I completed solving this last minute before the deadline hence didn't get time to discuss before raising the PR, apologies for the same.

Also, shifted the INFORMATION MANAGEMENT GROUP heading towards the left, so the page looks more presentable.

@AyushDharDubey
Copy link
Collaborator

Hi @disvid,

after discussing your PR, we have reached a consensus that the scope of issue #72 was broader than the current implementation. the issue was intended to include additional features such as tagging each ticket with a user, storing user info in mongodb clusters, implementing robust security measures such as token-based authentication, and enhancements like RBAC, etc.

given the initial scope, the issue was labeled as hard. however, since the PR was submitted at the last minute without prior discussion of the approach, the implementation flow is not as polished as it could be. now that the event has concluded and there is no time to incorporate all the required features, we have decided to adjust the difficulty level of the issue to medium.

we hope you understand this decision.

other than this,

  1. resolve the merge conflict.
  2. export the metadata object (line 13 in app/layout.jsx).
  3. add NEXTAUTH_SECRET to the env file and include it in the README.md file.

Thank you for participating in the event, we appreciate your contributions on the repo!

@disvid
Copy link
Collaborator Author

disvid commented Jan 14, 2025

Thank you @AyushDharDubey sir for reviewing and providing feedback on my PR. I appreciate the opportunity to contribute to the repository and be a part of this event.
1.I have resolved the merge conflicts.
2. exporting the metadata throws an error as 'exporting "metadata" from a component marked with "use client" is disallowed.'
3. I haven't used any NEXTAUTH_SECRET in .env.local and don't feel a requirement for the same.

Regarding the difficulty level of the issue, it would be great if you please let it remain hard , as implementing even this much part wasn't easy,and harder compared to other issues.
Rest, I completely understand and respect you final decision.
Thank you again for your guidance and the opportunity to contribute. I look forward to learning and improving further with IMG!

@AyushDharDubey
Copy link
Collaborator

regarding point 2, in that case, we can fall back to using basic html title and meta tags in a <head> component within app/layout.jsx and remove the metadata object altogether.

as for the environment variable NEXTAUTH_SECRET, is line 17 in app/api/auth/[...nextauth]/route.js unintended? you might like to take a second look into your codebase.

additionally, while resolving the merge conflict, it seems some previous review changes were inadvertently reverted. for eg, lines 64 and 65 of app/page.jsx need to be changed back to:

setTickets(data.tickets);  
setFilteredTickets(data.tickets);

and, i noticed there are two instances of INFORMATION MANAGEMENT GROUP in the navbar. I assume this is also a result of resolving merge conflicts untidily, please look into this duplication...

@disvid
Copy link
Collaborator Author

disvid commented Jan 14, 2025

@AyushDharDubey sorry sir, i have made the required changes.

@AyushDharDubey AyushDharDubey merged commit 5f2d4a2 into IMGIITRoorkee:master Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@disvid disvid

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Login functionality for ticketify via Google Oauth
3 participants
@disvid @rkcoder101 @AyushDharDubey