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

1178.accessing null values for non admin #1406

Draft
wants to merge 6 commits into
base: development
Choose a base branch
from

Conversation

BigGeF
Copy link
Contributor

@BigGeF BigGeF commented Dec 7, 2024

Description

This is a draft update focusing on changes to Meters-related files, with no modifications to Groups or Units. The goal is to validate this solution approach. we dicide not remove any optional chain.

Evan Thomas - @ethomas5

Test
Searched and replaced relevant optional chaining in the src/client/app directory.
No compile-time or console errors were found during testing of the affected pages.
Reason could be The existing “logic protection” like Meters.js

Key Changes
File Adjustments: Modified Meters files to ensure data structures are correctly asserted as "Admin" where applicable.

Significance

  1. Improved Reliability: Clarified type distinctions for Admin and Non-Admin roles, enhancing TypeScript type-checking and preventing potential runtime errors.
  2. Future-Ready: Prepares the codebase for future feature expansions requiring user permissions handling.
  3. Enhanced Readability: Refactored code improves clarity and reduces unnecessary handling of null values.

Partly Addresses #1178

Type of change

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

If this approach is approved, we will extend it to Groups and Units and submit a formal pull request.

@huss
Copy link
Member

huss commented Dec 10, 2024

Thanks to @BigGeF & @ethomas5 for working on this issue. I have looked it over and wanted to ask about your testing. The description talks about modifying the optional chaining and Meter files to see if this works. I don't see those changes so did you do this locally put not push to GitHub? It is okay but I want to understand. If you did that then could you let me have access to it since it would make my analysis/testing of these changes easier. Either to this PR or letting me know another location I can get the code. If not then let me know. If I have the wrong impression then just tell me.

From what I see it seems fine but I would like to see it working in the code base, per above, to be sure. Thanks.

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.

3 participants