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

GPII-4488 GPII-4497 Reverting back to saving all application settings, which li… #201

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

sgithens
Copy link
Member

…kely makes sense for capture anyways.

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/gpii-app-tests/1143/

@amb26
Copy link
Member

amb26 commented May 25, 2020

Please update this to the fresh version of gpii-windows including your other changes in 0.3.0-dev.20200525T101402Z.6e113cb

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/gpii-app-tests/1144/

@sgithens
Copy link
Member Author

looks like the VM itself didn't get created

ok to test

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/gpii-app-tests/1145/

@sgithens
Copy link
Member Author

@amb26 Ok, this is updated and tests seem to be passing in CI

@the-t-in-rtf
Copy link
Member

the-t-in-rtf commented May 29, 2020

I have some concerns about saving the application preferences that are the result of intra-application transforms, as we would be storing the value/path material from SPI settings and the value material from other settings handlers, rather than the cleaner user-facing settings we are transforming from.

For examples, just search win32.json5 for Config" (with a trailing quote). Unless we're purging or somehow distinguishing these, this commits us to working with value/path material until we have a versioning and migration strategy for preferences data.

This would make things like the SPI refactor much more painful, as it would require someone doing that to realise a versioning/migration strategy. It would also make it harder to silently standardise other settings handlers like the native settings handler and system settings handler, which use values but not paths.

Saving application-specific settings also completely undermines the remaining benefits of generic preference terms, even those used in the QSS, and limits the eventual ability to make portable payloads across platforms. Maybe this would be a good one for an overlap meeting on Tuesday?

@sgithens
Copy link
Member Author

Thanks @the-t-in-rtf . I agree we should meet about this, and I should do a bit more research on it as well...

@the-t-in-rtf
Copy link
Member

Per our discussion today, one solution longer term to greatly reduce our use of intra-application transforms is to clean up system settings handlers like this one and native settings handlers like this one.

@sgithens sgithens changed the title GPII-4488 Reverting back to saving all application settings, which li… GPII-4488 GPII-4497 Reverting back to saving all application settings, which li… Jun 5, 2020
@sgithens
Copy link
Member Author

sgithens commented Jun 5, 2020

Updating this based on work in GPII-4497, to which this work also applies.

@gpii-bot
Copy link

gpii-bot commented Jun 6, 2020

CI job passed: https://ci.gpii.net/job/gpii-app-tests/1160/

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/gpii-app-tests/1185/

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/gpii-app-tests/1186/

@amb26
Copy link
Member

amb26 commented Jun 17, 2020

Linting failure:

\\vboxsvr\vagrant\src\main\dialogs\captureToolDialog.js
  503:19  error  Missing semicolon  semi
  505:11  error  Missing semicolon  semi

@sgithens
Copy link
Member Author

Thanks @amb26 This should pass now hopefully. And in conjunction with GPII-4497 GPII/universal#882 I'm able to save all the SPI settings from the capture tool as application specific settings.

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/gpii-app-tests/1191/

@the-t-in-rtf
Copy link
Member

Provisioning failure in the last build. Ok to test again.

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/gpii-app-tests/1192/

@the-t-in-rtf
Copy link
Member

Actual test failure this go round:

03:00:10.902:  jq: FAIL: Module "gpii.tests.dev.config tests" Test name "Timer integration tests" - Message: Expected 3 assertions, but 4 were run

03:00:10.902:  jq: Source:     at Object.asyncTest (\\vboxsvr\vagrant\node_modules\infusion\tests\lib\qunit\js\qunit.js:406:9)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants