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

DDF-4429 Fixed shared workspace deletion reminder #4202

Merged
merged 4 commits into from
Jan 30, 2019

Conversation

criscrasbell
Copy link

What does this PR do?

  • Downstream projects can use the sharing component for other things than workspaces.
  • Fetches the current workspace before deletion to check for ACL changes.

Who is reviewing it?

@cantstoptheunk
@Corey-Collins

Select relevant component teams:

@codice/ui

Ask 2 committers to review/merge the PR and tag them here.

@andrewkfiedler
@djblue

How should this be tested?

  • Create a shared workspace and delete it.
  • Verify that the user receives a reminder.
  • Create a nonshared workspace and delete it.
  • Verify that the user does not receive a reminder.

Any background context you want to provide?

  • Downstream projects may want to use the sharing component for other types of metacards.

What are the relevant tickets?

Reusing this ticket number
[DDF-4429](https://codice.atlassian.net/browse/DDF- 4429)

Screenshots

Checklist:

  • Documentation Updated
  • Update / Add Threat Dragon models
  • Update / Add Unit Tests
  • Update / Add Integration Tests

Notes on Review Process

Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.

Review Comment Legend:

  • ✏️ (Pencil) This comment is a nitpick or style suggestion, no action required for approval. This comment should provide a suggestion either as an in line code snippet or a gist.
  • ❓ (Question Mark) This comment is to gain a clearer understanding of design or code choices, clarification is required but action may not be necessary for approval.
  • ❗ (Exclamation Mark) This comment is critical and requires clarification or action before approval.

@vinamartin vinamartin added the 👤 UI Has UI Changes label Jan 14, 2019
Copy link
Contributor

@andrewkfiedler andrewkfiedler left a comment

Choose a reason for hiding this comment

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

Do we need the extra call to the backend if we already have the workspace data?

If we do, we need to account for possible error responses, as well as response time.

@criscrasbell
Copy link
Author

@andrewkfiedler Originally the workspace model was being updated from sharing however the sharing component is generic enough to be used for other types of metacards so it has the potential to be used downstream for other things.

The reason we have to request the metacard is for the use case where sharing preferences are updated and then the user attempts to delete the workspace without a refresh in between.

@andrewkfiedler
Copy link
Contributor

@Bellcc if the user updates the sharing in app shouldn't we be able to update the sharing preferences at that point?

@criscrasbell
Copy link
Author

criscrasbell commented Jan 17, 2019

@andrewkfiedler Originally this is what this feature did however the sharing component is being used downstream with a different type of metacard. So we can't introduce workspace specific code necessarily.

We could have a control structure in sharing to update this (see pseudo code below) however I'm not a fan of the precedent that it sets. I think this component is better off being metacard type agnostic.

. . .

if (isWorkspace(metacard)) {
     // Update the store
}

. . .

@codice codice deleted a comment Jan 18, 2019
@andrewkfiedler
Copy link
Contributor

@Bellcc I think even the other models that reuse the sharing component probably need to be updated based on changes to the sharing though. Each component using it could pass in a callback that takes in the changes and uses those to update themselves as they see fit.

The alternative is going to be harder, where we would have to account for possible latency requesting the current state of sharing.

@criscrasbell
Copy link
Author

@andrewkfiedler I updated this PR so that it can properly handle sharing updates too. Since I modified the props for Sharing is this now a breaking change for any downstream projects that use Sharing?

@criscrasbell
Copy link
Author

build now

@cxbot
Copy link

cxbot commented Jan 29, 2019

Internal build has been scheduled, your results will be available at build completion.

@cxbot
Copy link

cxbot commented Jan 29, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins.codice.org/job/DDF-Jobs/job/pr/job/Linux/6135/
✅ JOB SUCCESS

@criscrasbell
Copy link
Author

build now

@cxbot
Copy link

cxbot commented Jan 30, 2019

Internal build has been scheduled, your results will be available at build completion.

@cxbot
Copy link

cxbot commented Jan 30, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins.codice.org/job/DDF-Jobs/job/pr/job/Linux/6139/
✅ JOB SUCCESS

@andrewzimmer
Copy link
Contributor

Hero Successful 🎉

  • verified user receives notification for deleting a shared workspace
  • verified no notification for deleting a non-shared workspace
  • verified search forms can be successfully shared and saved

@bdeining bdeining merged commit a8bb213 into codice:master Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👤 UI Has UI Changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants