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

disabling GUI buttons of forced External Storage #1181

Merged
merged 6 commits into from
Nov 14, 2023

Conversation

paulr34
Copy link
Collaborator

@paulr34 paulr34 commented Oct 25, 2023

This PR addresses both issues:
#1086,
#1145

If attribute storage policy is attributeAccessInterface then the storage and the default field are not editable. It also sets default value to null.

The default field is still there but it is disabled.

@paulr34 paulr34 marked this pull request as ready for review November 8, 2023 16:54
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (1428d6e) 65.43% compared to head (bce50bb) 65.48%.

Files Patch % Lines
src-electron/matter/matter.js 18.18% 9 Missing ⚠️
src-electron/rest/user-data.js 37.50% 5 Missing ⚠️
src/components/ZclAttributeManager.vue 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1181      +/-   ##
==========================================
+ Coverage   65.43%   65.48%   +0.04%     
==========================================
  Files         185      185              
  Lines       19104    19124      +20     
  Branches     4092     4093       +1     
==========================================
+ Hits        12501    12523      +22     
+ Misses       6603     6601       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brdandu
Copy link
Collaborator

brdandu commented Nov 8, 2023

Seeing a lot of changes to the matter code gen. Is this expected? See all .matter files with diffs.

Copy link
Collaborator

@brdandu brdandu left a comment

Choose a reason for hiding this comment

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

Add tests to show that when external storage option is used then only Matter is affected and not Zigbee

@paulr34
Copy link
Collaborator Author

paulr34 commented Nov 8, 2023

Seeing a lot of changes to the matter code gen. Is this expected? See all .matter files with diffs.

Yes, they look right. For example, in Access Control we see

  • callback attribute clusterRevision default = 1;
  • callback attribute clusterRevision;

the default for this external attribute is set to null

@paulr34
Copy link
Collaborator Author

paulr34 commented Nov 8, 2023

Add tests to show that when external storage option is used then only Matter is affected and not Zigbee

Zigbee doesn't have attributeAccessInterface

Good catch, I made this commit to limit it to Matter data 3bc7a3a

As long as Zigbee does not have attributeAccessInterface in their zcl.json then it won't effect it but it would definitely be a good idea to add a cypress test

@paulr34 paulr34 mentioned this pull request Nov 9, 2023
@@ -25,7 +25,7 @@ const dbMapping = require('./db-mapping.js')
const queryPackage = require('./query-package.js')
const dbEnum = require('../../src-shared/db-enum.js')
const queryZcl = require('./query-zcl.js')
const queryUpgrade = require('../upgrade/upgrade.js')
const queryUpgrade = require('../matter/matter.js')
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a matter of general rules: we should NOT use the .js suffix in require calls.
That allows you to migrate the specific file to a different language (like TS) without having to modify these requires.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay ill make a new PR for going through and fixing that for all the files

@@ -108,7 +109,8 @@ function httpGetPackageNotifications(db) {
function httpGetPackageNotificationsByPackageId(db) {
return async (request, response) => {
let packageId = request.params.packageId
let notifications = await queryPackageNotification.getNotificationByPackageId(db, packageId)
let notifications =
await queryPackageNotification.getNotificationByPackageId(db, packageId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have these formatting diffs? Do we not all use the same formatter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure

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.

4 participants