Fix/validation errors admin fragment#51
Open
siddhigupta075 wants to merge 5 commits intoiiitl:masterfrom
Open
Conversation
1 task
WalkthroughAdmin page UI and behavior updated: view binding made nullable and cleared in onDestroyView; email and designation validation added; add operation disables button and shows "Adding..." while calling ViewModel which returns success/failure; multiple drawable and layout styling changes added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AdminFragment as Fragment
participant AdminVM as ViewModel
participant Firestore
User->>Fragment: fills email + designation, taps Add
Fragment->>Fragment: validateAndAdd() (trim, validate)
alt valid inputs
Fragment->>Fragment: disable button, set text "Adding..."
Fragment->>AdminVM: addToMessCommittee(email, designation)
AdminVM->>Firestore: update(...) (async)
Firestore-->>AdminVM: success / failure
AdminVM-->>Fragment: onResult(success|error)
Fragment->>Fragment: re-enable button, restore text
alt success
Fragment->>Fragment: clear input fields, show success toast
else error
Fragment->>Fragment: show toast with error
end
else invalid
Fragment->>Fragment: show inline field errors
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/theayushyadav11/MessEase/ui/NavigationDrawers/Fragments/AdminFragment.kt`:
- Around line 86-88: The code is clearing the input fields unconditionally after
any toast; change AdminFragment's handler that currently calls
mess.toast(result) followed by binding.etEmail.text?.clear() and
binding.spinnerAutoComplete.text?.clear() so it only clears when the ViewModel
indicates a real success (e.g., the success message string or a success flag
from AdminViewModel). Locate the callback/observer in AdminFragment that
receives the result from AdminViewModel (the place that calls
mess.toast(result)) and add a conditional: if result indicates success (or check
the success boolean from the ViewModel), then clear binding.etEmail and
binding.spinnerAutoComplete; otherwise leave user input intact.
- Around line 73-84: The UI cleanup and button state reset must run on every
completion path: move the progress dismiss call (mess.pbDismiss()) so it
executes before the `_binding` null-check in the viewModel.addToMessCommittee
callback and ensure you always re-enable `binding.btnAdd` and reset its text
even if `_binding` is null; additionally update
`AdminViewModel.addToMessCommittee` so it invokes the provided completion lambda
(onResult) for both success and failure/error listeners (not just success) so
the caller always gets notified to unwind the loading UI.
- Around line 93-99: The callback passed to Mess.getLists(...) can run after the
fragment view is destroyed; guard it by checking fragment and view state before
using requireContext() or binding: inside the lambda, replace direct calls with
a safe check like if (!isAdded || binding == null || context == null) return@...
(or use context?.let and val b = binding ?: return@...) and only then create the
ArrayAdapter and call binding.spinnerAutoComplete.setAdapter(adapter); reference
Mess.getLists, AdminFragment (the surrounding method), and
binding.spinnerAutoComplete when applying the guard.
In `@app/src/main/res/drawable/baseline_attach_email_24.xml`:
- Around line 2-5: Remove any app:tint attributes on <path> elements (attribute
name: app:tint) and instead use android:fillColor on those <path> tags if they
need specific colors; leave the root vector's android:tint in place to tint the
whole drawable. Also remove the xmlns:app namespace declaration if no other app:
attributes remain. Apply the same change to the other occurrences noted (the
other <path> entries).
In `@app/src/main/res/layout/fragment_admin.xml`:
- Around line 51-75: The divider view is currently constrained to
app:layout_constraintTop_toBottomOf="@id/ivHeaderIcon" which can cause overlap
when the title/subtitle wrap; change the divider's top constraint to
app:layout_constraintTop_toBottomOf="@id/subtitle" (reference the divider view
id "divider" and subtitle id "subtitle") so the divider always sits below the
subtitle; keep existing horizontal constraints and margins as-is to preserve
spacing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69cd8141-c4c8-435a-842b-a7982d392e44
📒 Files selected for processing (7)
app/src/main/java/com/theayushyadav11/MessEase/ui/NavigationDrawers/Fragments/AdminFragment.ktapp/src/main/res/drawable/baseline_attach_email_24.xmlapp/src/main/res/drawable/baseline_person_24.xmlapp/src/main/res/drawable/bg_card_glass_inner.xmlapp/src/main/res/drawable/bg_gradient_full.xmlapp/src/main/res/drawable/bg_icon_soft.xmlapp/src/main/res/layout/fragment_admin.xml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/theayushyadav11/MessEase/ui/NavigationDrawers/ViewModels/AdminViewModel.kt (1)
15-33:⚠️ Potential issue | 🟠 MajorAdd error handling for the Firestore lookup query to prevent caller from hanging.
The
whereEqualTo(EMAIL, email).get()call on line 6 only hasaddOnSuccessListener. If the query fails (network error, permission denied),onResultis never called and the caller can remain stuck in a submitting state indefinitely.Suggested patch
- firestoreReference.collection(USERS).whereEqualTo(EMAIL, email).get().addOnSuccessListener { + firestoreReference.collection(USERS).whereEqualTo(EMAIL, email).get() + .addOnSuccessListener { if (it.isEmpty) { onResult("User not found") } else { val id = it.documents[0].id val updates = mapOf( DESIGNATION to designation, IS_MEMBER to true ) firestoreReference.collection(USERS).document(id).update(updates) .addOnSuccessListener { onResult("User added to committee") } .addOnFailureListener {e -> onResult(e.message ?: "Error adding user to committee") } } - } + } + .addOnFailureListener { e -> + onResult(e.message ?: "Error finding user") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/theayushyadav11/MessEase/ui/NavigationDrawers/ViewModels/AdminViewModel.kt` around lines 15 - 33, The Firestore lookup using firestoreReference.collection(USERS).whereEqualTo(EMAIL, email).get() only attaches addOnSuccessListener so onResult is never called on query failure; add an addOnFailureListener (or addOnCompleteListener with success/failure handling) to the initial get() call to invoke onResult with a meaningful error string (e.g., e.message ?: "Error querying user") so the caller is always notified; update the block around whereEqualTo/ get() to attach the failure handler and ensure any exceptions from document update (firestoreReference.collection(USERS).document(id).update) are already covered by its addOnFailureListener.
🧹 Nitpick comments (1)
app/src/main/java/com/theayushyadav11/MessEase/ui/more/SettingsActivity.kt (1)
28-31: Prefer zipping keys/defaults instead of index-based access.Line 30 depends on positional indexing across two lists. It works now, but
zipavoids accidental index drift and is safer to maintain.♻️ Suggested refactor
- val defaultTimes = listOf("7:30", "12:0", "16:30", "19:0") - val times = mealKeys.mapIndexed { index, key -> - mess.get(key, defaultTimes[index]) - } + val defaultTimes = listOf("7:30", "12:0", "16:30", "19:0") + val times = mealKeys.zip(defaultTimes).map { (key, defaultTime) -> + mess.get(key, defaultTime) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/theayushyadav11/MessEase/ui/more/SettingsActivity.kt` around lines 28 - 31, The code uses mapIndexed with positional indexing (mealKeys.mapIndexed { index, key -> mess.get(key, defaultTimes[index]) }) which can break if lists diverge; replace that with zipping mealKeys with defaultTimes and mapping the pairs, e.g. zip mealKeys and defaultTimes then for each (key, default) call mess.get(key, default) so the mapping of keys to defaults is explicit and safe (refer to the variables mealKeys, defaultTimes and the mess.get call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/src/main/java/com/theayushyadav11/MessEase/ui/NavigationDrawers/ViewModels/AdminViewModel.kt`:
- Around line 15-33: The Firestore lookup using
firestoreReference.collection(USERS).whereEqualTo(EMAIL, email).get() only
attaches addOnSuccessListener so onResult is never called on query failure; add
an addOnFailureListener (or addOnCompleteListener with success/failure handling)
to the initial get() call to invoke onResult with a meaningful error string
(e.g., e.message ?: "Error querying user") so the caller is always notified;
update the block around whereEqualTo/ get() to attach the failure handler and
ensure any exceptions from document update
(firestoreReference.collection(USERS).document(id).update) are already covered
by its addOnFailureListener.
---
Nitpick comments:
In `@app/src/main/java/com/theayushyadav11/MessEase/ui/more/SettingsActivity.kt`:
- Around line 28-31: The code uses mapIndexed with positional indexing
(mealKeys.mapIndexed { index, key -> mess.get(key, defaultTimes[index]) }) which
can break if lists diverge; replace that with zipping mealKeys with defaultTimes
and mapping the pairs, e.g. zip mealKeys and defaultTimes then for each (key,
default) call mess.get(key, default) so the mapping of keys to defaults is
explicit and safe (refer to the variables mealKeys, defaultTimes and the
mess.get call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 756c9bd5-354e-431b-8fa9-8139baa1c7af
📒 Files selected for processing (6)
app/src/main/java/com/theayushyadav11/MessEase/ui/NavigationDrawers/Fragments/AdminFragment.ktapp/src/main/java/com/theayushyadav11/MessEase/ui/NavigationDrawers/ViewModels/AdminViewModel.ktapp/src/main/java/com/theayushyadav11/MessEase/ui/more/SettingsActivity.ktapp/src/main/res/drawable/baseline_attach_email_24.xmlapp/src/main/res/layout/fragment_review.xmlapp/src/main/res/values-night/themes.xml
✅ Files skipped from review due to trivial changes (3)
- app/src/main/res/layout/fragment_review.xml
- app/src/main/res/drawable/baseline_attach_email_24.xml
- app/src/main/res/values-night/themes.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/theayushyadav11/MessEase/ui/NavigationDrawers/Fragments/AdminFragment.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #49
Description
This PR fixes validation issues in
AdminFragment.Earlier, validation was done using a
whenblock, so only the first error was shown.Also, old error messages were not cleared, which caused incorrect errors to remain visible even after fixing inputs.
Now:
This improves user experience and makes validation behave correctly.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes / Stability
Style
Refactor