Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the admin users index view to better handle ORCID tokens when the token expiration timestamp is missing, preventing errors on the admin users route.
Changes:
- Add a “No expiration” display state when
user.orcid_expires_atis nil. - Guard the “delete token” action rendering with a presence check and parse
orcid_expires_atbefore comparing toTime.zone.now.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <span class="title pull-left"><%= link_to_if user.claims_count > 0, pluralize(user.claims_count, "Claim"), admin_claims_path(source: params[:source], state: params[:state], user_id: user.uid) %></span> | ||
| <div class="btn-toolbar"> | ||
| <% if can?(:manage, user) && Time.zone.now < user.orcid_expires_at %> | ||
| <% if can?(:manage, user) && user.orcid_expires_at.present? && Time.zone.now < DateTime.parse(user.orcid_expires_at) %> |
There was a problem hiding this comment.
can?(:manage, user) is checked twice: the whole footer is already inside <% if can?(:manage, user) %>, so repeating the same authorization check in the nested condition adds noise and makes future edits easier to get wrong. You can simplify the inner conditional to only the ORCID expiry/token logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <% if user.orcid_expires_at.present? && Time.zone.now < DateTime.parse(user.orcid_expires_at) %> | ||
| <div class="btn-group btn-group-sm pull-right"> | ||
| <%= link_to 'delete token', admin_user_path(user.uid, user: { orcid_token: nil, orcid_expires_at: Time.zone.now }), { method: :put, remote: true, class: 'btn btn-sm btn-warning btn-fill' } %> |
There was a problem hiding this comment.
The "delete token" button is now gated on user.orcid_expires_at.present?, which means admins can no longer delete tokens that exist but have a nil expiration (the UI even shows "No expiration" above). Consider changing the condition to allow deletion whenever a token is present, and only use the expiration timestamp to decide whether to label it expired (rather than hiding the action entirely).
| <% elsif user.orcid_expires_at.nil? %> | ||
| No expiration | ||
| <% elsif DateTime.parse(user.orcid_expires_at) < Time.zone.now %> | ||
| Expired | ||
| <% else %> |
There was a problem hiding this comment.
DateTime.parse(user.orcid_expires_at) is now being called in multiple branches (and again in the footer condition). To reduce repeated parsing and avoid type-mismatch surprises when comparing with Time.zone.now, consider parsing once into a local variable (preferably using Time.zone.parse/Time.iso8601 depending on the stored format) and reusing it for both comparisons and formatting.
Purpose
closes: Add github issue that originated this PR
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines: