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

Effective policies isn't updated correctly in some situations #982

Closed
jabrah opened this issue Jul 3, 2019 · 3 comments · Fixed by #989
Closed

Effective policies isn't updated correctly in some situations #982

jabrah opened this issue Jul 3, 2019 · 3 comments · Fixed by #989
Assignees
Labels
blocker critical task - needs to be worked on before launch bug

Comments

@jabrah
Copy link
Contributor

jabrah commented Jul 3, 2019

  • Start a submission, add an NIH grant and a non-NIH grant
  • Go through workflow until you get to the metadata forms
  • Go back to Grants step and remove the NIH grant
  • Go through the workflow
    • At the Repositories step, the UI would error out and reload

This bug occurred because the UI was not updating submission.effectivePolicies when grants were selected or removed. Instead only the policy cards themselves were able to add the remove themselves. This was done initially to allow users to remove the NIH policy by selecting the option saying that the publisher had/has taken care of the submission. However, the policy step would not create an NIH card that could remove the policy from effectivePolicies because the NIH grant was no longer associated with the submission. Since there was now an erroneous "effective policy" that could no longer be removed, the policy service saw this and threw an error, as it should.

There is an a somewhat error in the Repositories step where current repositories were not being updated if repositories were already associated with the submission. This meant that if a user had gone through the workflow and set one or more repositories, but then later went back and added or removed a grant (associated with a different repository), then the action would not have any consequences on the repositories associated with the submission.

The two bugs described above are both related to the fact that changes to grants in an in-progress submission isn't currently propagated properly through the submission object.

PS sorry about how verbose this is getting - I just need to organize my thoughts here

@jabrah jabrah added bug blocker critical task - needs to be worked on before launch labels Jul 3, 2019
@jabrah jabrah self-assigned this Jul 3, 2019
@jabrah
Copy link
Contributor Author

jabrah commented Jul 3, 2019

The issue with effective policies should only be an issue when removing a grant, but the repository issue may be a problem for adding and removing grants. The repository step must be updated either way to allow adding/removing repositories when needed. Because of this, it is really only removing grants that becomes problematic. At the moment, I see two solutions to this issue.

  • We can either inherently trust the policy service to give us correct data and adjust data in the submission object to fit policy service results (PR 983 goes this route)
  • We could front load data modification logic as soon as the user changes the grants selection. A function in the submission-handler could be crafted to propagate changes as necessary
    • Logic could be added to selectively remove effective policies and repositories from the submission that match to the grant that was removed
    • Simply clear effective policies and repositories in the submission when the user makes a change to grants and let them be recalculated when the user gets to the appropriate step in the workflow. This problem only exists in draft submissions, so there wouldn't be any external URIs or anything worth keeping.

@jabrah
Copy link
Contributor Author

jabrah commented Jul 4, 2019

To add more detail about an issue with the repository step:

The Repository step will have no notion of a "new" or just-added repository. With the current way things are processed, optional repositories are presented by the policy service to the user. If the user decides to not use those optional repositories, they will not be added to draft submission. This leaves a problem in this component of trying to determine the difference between (1) an "old" repository that was not selected and thus not added to the repository, and (2) a repository for a newly added grant. Both of these types of repositories would appear in the policy service results under the optional field and neither would be present on submission.repositories.

For (1), we would always want to render this to the user as an unchecked checkbox. However, for (2), we would want the status of the checkbox to be set to whatever the default selected status was in the policy service result.

Note that this is "just" a usability issue from a user perspective for what would hopefully be an edge case.

Can we live with one of the above options (ignore default selection, or ignore possible previous user input)? Or should this be more fully addressed? Is this even an issue or am I just overthinking things :)

This seems to be in the same class of issues as #948, that being UI state issues because we don't persist UI state with draft submissions.

@jabrah
Copy link
Contributor Author

jabrah commented Jul 10, 2019

For now, let's assume that we can't persist UI state data. What data do we have from the user being forced to go through the workflow?

  • submission.repositories
  • policyServicec#repositories response
  • We can also infer some things from previous steps in the workflow

If an optional/choice repository is NOT present in submission.repositories, that could mean either:

  • The user did not select or explicitly unselected the repository in a previous pass through the workflow
  • The user added a new grant to the submission this pass through the workflow that brought one or more new optional/choice repositories

How do we differentiate between these two situations?

  • Track the grants that were added in workflow.js - which will keep this list though a single time through the workflow
  • Associate these added grants with Policies/Repositories based on responses from the Policy service. How do we get this mapping?
    • Infer based on model objects we have access to: grant.*Funder.policy.repositories
    • A 2nd set of calls to the Policy service with the select grants (actually wouldn't work because the policy service reads a Submission object from Fedora)
  • During Repository Step init:
    • Check for added repositories in workflow.js
      • If present, use the repository's default selection status
      • If NOT present, derive selection state from the repository's presence in submission
        • If present in submission: repository._selected = true
        • else: repository._selected = false

Repositories Step init:

Prerequisites: workflow.js tracks added grants/funders/repos

  • Call policyService#repositories to get all applicable repositories
  • Check for repositories added to workflow from earlier in the workflow
    • If present, set _selected property for all optional and choice repositories based on the default selection status. This status is found in the policy service response
    • If NOT present and
      • Present in submission.repositories, repository._selected = true
      • Not present in submission.repositories, repository._selected = false

Instead of using workflow to hold some state, we should probably use submission/new route

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker critical task - needs to be worked on before launch bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant