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

[issue_tracker] Update validation to allow NULL Site #6526

Merged

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented May 12, 2020

Brief summary of changes

Since we want to allow new issues with NULL centerID to be created, the following changes have been made:

Add/Edit issue

  • a new Site option All Sites allows a user to save an issue with a NULL site
  • The Comment History section displays site to All sites if an issue is edited/modified with the All Sites option.
  • Alternatively, in the Comment History section, All sites can be replaced with a comma separated list of all CenterID (to discuss)

Index

  • An issue with a null centerID displays All Sites in the site column
  • Issues can be filtered by sites: All Sites.
  • Alternatively, All sites can be replaced with a comma separated list of all CenterID (to discuss) in the datatable column, and sites can be made multi-selectable.

Test plan

Linked Issues

@laemtl laemtl added 23.0.0-testing Category: Feature PR or issue that aims to introduce a new feature labels May 12, 2020
@driusan
Copy link
Collaborator

driusan commented May 13, 2020

does this need a test plan update?

@cmadjar
Copy link
Collaborator

cmadjar commented May 15, 2020

@laemtl Could you update the test plan accordingly? Thank you!

@laemtl
Copy link
Contributor Author

laemtl commented May 15, 2020

@driusan, @cmadjar Sure!

@laemtl laemtl force-pushed the 2020-05-12-new-issues-null-centerID branch from 6ff3e2a to ca1fe3b Compare May 15, 2020 20:24
Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Thanks Laetitia. good catch @cmadjar re the test plan. I've suggested a little more granularity in the update to the test plan.
Laetitia does this make sense to you, and could you fill in what is supposed to happen on the new testing point 7 ( see my suggestion/comment) -- merci !

Comment on lines 16 to 20
5. Submit invalid and valid PSCID and visit label pairs. Error messages should respond accordingly. Not that you cannot submit PSCIDs from other sites unless you have access all profiles permission
6. Submit just a visit label - this should give an error message.
7. Check that all values are propagated and saved correctly.
8. Add an attachment to the new issue and make sure that it is successfully uploaded.
9. Check that watching logging is working - turn it off and on for your current user, and for other watchers on the issue
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
5. Submit invalid and valid PSCID and visit label pairs. Error messages should respond accordingly. Not that you cannot submit PSCIDs from other sites unless you have access all profiles permission
6. Submit just a visit label - this should give an error message.
7. Check that all values are propagated and saved correctly.
8. Add an attachment to the new issue and make sure that it is successfully uploaded.
9. Check that watching logging is working - turn it off and on for your current user, and for other watchers on the issue
5. Submit invalid and valid PSCID and visit label pairs. Error messages should display accordingly.
6. A user should be able to submit a PSCID from other sites only if they have access_all_profiles permission.
7. Submit a PSCID and leave the Site blank _ .. this should/not work if... _
8. Submit just a visit label - this should give an error message.
9. Check that all values are propagated and saved correctly.
10. Add an attachment to the new issue and make sure that it is successfully uploaded.
11. Check that watching logging is working - turn it off and on for your current user, and for other watchers on the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking down testing point 5.

  • To do : Point 7 - needs to be completed with what should be happening when a PSCID is entered with Null site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christinerogers Are 7. and 3. redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point.
3 could specify that PSCID is left blank.
7 tests the case where there is a PSCID, and Site doesn't match or is null.

Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Thanks @laemtl

@@ -6,16 +6,20 @@ Issue Tracker Filter Form [Automation Testing]
5. Test that the watching checkbox works correctly (issues that your userID is watching in issues_watching table)
6. Check that links to issues in table are correct.
7. Check that table sorts and displays additional pages correctly
8. Check that a user who does not have access to all centers can see all issues with a site set to NULL.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should say what permission it's referring to, since we have like 20 different access all site permissions..

@laemtl laemtl force-pushed the 2020-05-12-new-issues-null-centerID branch 4 times, most recently from 9f02f64 to 43a2fa0 Compare May 19, 2020 20:26
Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Refining your change after dave's comment - see my comment about how User Accounts is used for this purpose (and this doesn't need to be explained).
Otherwise LGTM 👍

modules/issue_tracker/test/issue_tracker_test_plan.md Outdated Show resolved Hide resolved
@laemtl laemtl requested a review from christinerogers May 22, 2020 18:38
@christinerogers
Copy link
Contributor

failing travis 😢 i know alex and shen are working on this

Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

thanks Laetitia, looks great to me.

  • This will need to be tested in Round 3 of 23 release testing, since it hasn't been merged/pulled onto the Test Vm for Round 2.

@laemtl laemtl force-pushed the 2020-05-12-new-issues-null-centerID branch from 0ab8463 to 6fc6548 Compare May 25, 2020 22:08
Copy link
Collaborator

@HenriRabalais HenriRabalais left a comment

Choose a reason for hiding this comment

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

Perhaps to make it more intuitive the user, we should replace the empty option in the drop down to a string that says "All" to make it clear that by not selecting a site, they are in fact selecting all sites.

@christinerogers
Copy link
Contributor

Perhaps to make it more intuitive the user, we should replace the empty option in the drop down to a string that says "All" to make it clear that by not selecting a site, they are in fact selecting all sites.

👍

This used to be the practice across all module filters before Reactification, or at least many filters. @HenriRabalais Do you remember why we discontinued?

@HenriRabalais
Copy link
Collaborator

@christinerogers iirc that was because it made the filter feel really busy with values that didn't need to be there. For filters, it's implied that if there's no value in the field, it's not filtering on that field (no value in filter field == no filtering for that field == all values for that field are presented).

But for forms I don't think that same implication holds. It's expected that an empty field literally means that the value submitted will be NULL. If in this particular case for centerIds we are taking a NULL value in the back end to mean "All Sites", then I think the "All Sites" part is what should be communicated to the user on the front end. This way, the field would still be required, but they would have the option of selecting "All Sites".

@christinerogers
Copy link
Contributor

Thanks @HenriRabalais, good points - I wonder what the odds are we'll get a user confused because they entered an issue for "All Sites" but can't filter on that literal value.

@driusan
Copy link
Collaborator

driusan commented Jun 1, 2020

@HenriRabalais GitHub says you requested changes but it's not clear from the thread what changes you want her to make on this PR.. is there something outstanding?

Regarding the underlying issue, what if there was an emptyValue=string prop (or similar) so that forms could set the label they want for the empty value without affecting all of our filters?

Copy link
Collaborator

@HenriRabalais HenriRabalais left a comment

Choose a reason for hiding this comment

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

@driusan I'll make requested changes more clear:

I think that CenterID should remain a required field for submission, however an option should be available for the user that displays as "All Sites", but submits a NULL value to be stored in the datatable.

@driusan driusan added the State: Needs work PR awaiting additional work by the author to proceed label Jun 2, 2020
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

see Henri's review

@laemtl laemtl force-pushed the 2020-05-12-new-issues-null-centerID branch 2 times, most recently from 4350997 to 6264ad0 Compare June 15, 2020 06:21
@laemtl laemtl requested a review from HenriRabalais June 16, 2020 18:13
@laemtl laemtl force-pushed the 2020-05-12-new-issues-null-centerID branch from 6264ad0 to de317ae Compare June 16, 2020 18:32
@laemtl laemtl changed the base branch from master to 23.0-release June 16, 2020 18:33
@laemtl laemtl force-pushed the 2020-05-12-new-issues-null-centerID branch from de317ae to ab5fdc9 Compare June 16, 2020 18:44
@laemtl
Copy link
Contributor Author

laemtl commented Jun 17, 2020

@HenriRabalais I updated the code following your suggestion.

@laemtl laemtl force-pushed the 2020-05-12-new-issues-null-centerID branch 4 times, most recently from 988cae5 to c02e59a Compare June 18, 2020 00:44
Copy link
Collaborator

@HenriRabalais HenriRabalais left a comment

Choose a reason for hiding this comment

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

Looks good! just some left over console log. I'll test out the PR and give it an approval if it passes.

result = <td>{this.state.data.fieldOptions.sites[cell]}</td>;
// if cell is an array containing all sites values
console.log(this.state.data.centerIDs);
console.log(cell);
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like some forgotten console logs

@laemtl laemtl force-pushed the 2020-05-12-new-issues-null-centerID branch from c02e59a to e046121 Compare June 18, 2020 14:16
@laemtl laemtl requested a review from HenriRabalais June 18, 2020 14:17
@ridz1208
Copy link
Collaborator

ridz1208 commented Jul 2, 2020

@driusan @HenriRabalais please re-review

Copy link
Collaborator

@HenriRabalais HenriRabalais left a comment

Choose a reason for hiding this comment

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

I think this does what it aims to do!

@driusan driusan dismissed their stale review July 7, 2020 18:24

stale review

@driusan driusan merged commit 382f308 into aces:23.0-release Jul 8, 2020
@ridz1208 ridz1208 added this to the 23.0.2 milestone Jul 15, 2020
spell00 pushed a commit to spell00/Loris that referenced this pull request Aug 13, 2020
Add ability to set a site to null to denote "all sites" in the issue tracker. Also update test plan both to handle this case and reintroduce Attachment testing instruction deleted by accident in aces#6636

    Resolves aces#6518
spell00 pushed a commit to spell00/Loris that referenced this pull request Aug 13, 2020
Add ability to set a site to null to denote "all sites" in the issue tracker. Also update test plan both to handle this case and reintroduce Attachment testing instruction deleted by accident in aces#6636

    Resolves aces#6518
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 15, 2021
Add ability to set a site to null to denote "all sites" in the issue tracker. Also update test plan both to handle this case and reintroduce Attachment testing instruction deleted by accident in aces#6636

    Resolves aces#6518
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 15, 2021
Add ability to set a site to null to denote "all sites" in the issue tracker. Also update test plan both to handle this case and reintroduce Attachment testing instruction deleted by accident in aces#6636

    Resolves aces#6518
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Sep 2, 2021
Add ability to set a site to null to denote "all sites" in the issue tracker. Also update test plan both to handle this case and reintroduce Attachment testing instruction deleted by accident in aces#6636

    Resolves aces#6518
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[issue_tracker] Update validation to allow NULL Site
7 participants