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

fix(hub-sites): prevent surveys and their feature services from being… #669

Merged

Conversation

rweber-esri
Copy link
Contributor

… shared to site teams during si

affects: @esri/hub-sites, @esri/hub-surveys

ISSUES CLOSED: 2325

  1. Description:

  2. Instructions for testing:

  3. Closes Issues: #2325

  4. ran commit script (npm run c)

For more information see the README

… shared to site teams during si

affects: @esri/hub-sites, @esri/hub-surveys

ISSUES CLOSED: [2325](https://devtopia.esri.com/dc/hub/issues/2325)
Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

Nice work @rweber-esri, but I would like to keep from introducing another peer dep, which would require a breaking change and add to the work in #655

packages/sites/package.json Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #669 (1c33520) into master (bcc380c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #669   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          462       457    -5     
  Lines         6623      6642   +19     
  Branches      1053      1058    +5     
=========================================
+ Hits          6623      6642   +19     
Impacted Files Coverage Δ
...mon/src/surveys/get-input-feature-service-model.ts 100.00% <ø> (ø)
...t-source-feature-service-model-from-fieldworker.ts 100.00% <ø> (ø)
...ckages/common/src/surveys/get-stakeholder-model.ts 100.00% <ø> (ø)
packages/common/src/surveys/is-fieldworker-view.ts 100.00% <ø> (ø)
packages/surveys/src/sharing/set-access.ts 100.00% <ø> (ø)
packages/common/src/index.ts 100.00% <100.00%> (ø)
packages/common/src/surveys/get-survey-models.ts 100.00% <100.00%> (ø)
packages/common/src/surveys/index.ts 100.00% <100.00%> (ø)
packages/sites/src/_get-sharing-eligible-models.ts 100.00% <100.00%> (ø)
packages/sites/src/_share-items-to-site-groups.ts 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcc380c...1c33520. Read the comment docs.

@rweber-esri
Copy link
Contributor Author

rweber-esri commented Oct 15, 2021

Nice work @rweber-esri, but I would like to keep from introducing another peer dep, which would require a breaking change and add to the work in #655

@tomwayson I believe I've addressed your feedback. I:

  • Added a surveys dir in hub-common
  • Moved the method/file/tests/fixtures (and their dependencies) that need to be consumed from hub-sites to the new dir in hub-common
  • Renamed the fixtures that moved to .json vs .ts to be more consistent w/ other fixtures in hub-common, updated all refs to these fixtures to account for the change
  • Re-exported things that moved to hub-common from hub-surveys to prevent breaking changes.

import { StakeholderItem } from "../mocks/stakeholder-item";
import { UpdateGroup } from "../mocks/update-group";
import { ViewGroup } from "../mocks/view-group";
import * as FormItemDraft from "../../../common/test/mocks/items/form-item-draft.json";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomwayson wasn't sure how else to re-use the fixtures that moved from hub-surveys to hub-common short of just duplicating them in hub-surveys and hub-common. I think this is OK since it's just tests, and Node runs them, so it's able to resolve the paths even though it's reaching out of the current package. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. What I did for other fixtures was to make a symlink under the consuming package's test folder to the fixtures under common.

@rweber-esri
Copy link
Contributor Author

@tomwayson was having some issues w/ GH and did not explicitly add approval as a result, but did give me verbal approval this is good-to-go.

@rweber-esri rweber-esri merged commit 38a7dcc into master Oct 15, 2021
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.

2 participants