Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Battery] Potential improvement for battery assignment #7163

Merged
merged 3 commits into from
Dec 23, 2020

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Dec 2, 2020

Brief summary of changes

The behaviour change in the battery was copied over from the assign_missing _instruments php script. This is not the ideal implementation but is sufficient to serve as a proposal up for discussion. Alternate solutions can include

  • completely separate function to populate vs create a battery
  • only allow to "fill in" battery if already existing instruments are surveys (not a prepopulated battery of instruments)
  • behaviour could be added uniquely to next stage (although this limits the potential to allow users to have a button au run "assign missing instruments from the front end")
  • ... other combinations...

fixes #7162

@ridz1208 ridz1208 added the Proposal PR or Issue suggesting an improvement that can be accepted, rejected or altered label Dec 2, 2020
@ridz1208
Copy link
Collaborator Author

ridz1208 commented Dec 2, 2020

@sruthymathew123 this is based on 21 so it should work as is, give it a try ?

@sruthymathew123
Copy link
Contributor

@ridz1208 Tested on IBIS and works well.
I really appreciate your time to look on this.

@sruthymathew123 sruthymathew123 added the Passed Manual Tests PR has undergone proper testing by at least one peer label Dec 2, 2020
@ridz1208 ridz1208 added the Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties label Dec 3, 2020
@ridz1208 ridz1208 changed the base branch from 21.0-release to 23.0-release December 22, 2020 19:14
@ridz1208 ridz1208 removed Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties Proposal PR or Issue suggesting an improvement that can be accepted, rejected or altered labels Dec 23, 2020
@ridz1208
Copy link
Collaborator Author

@CamilleBeau you reported this in #7162 can you confirm this fix works for you ?

@CamilleBeau
Copy link
Contributor

CamilleBeau commented Dec 23, 2020

@CamilleBeau you reported this in #7162 can you confirm this fix works for you ?

Just tested this on the 23 upgrade branch on CCNA. No longer getting the 500 error, but now the rest of the instruments other than the added surveys are not populating in the battery. Is that intended?

@ridz1208
Copy link
Collaborator Author

@CamilleBeau def not

@CamilleBeau
Copy link
Contributor

Looks good!

@ridz1208 ridz1208 closed this Dec 23, 2020
@ridz1208 ridz1208 reopened this Dec 23, 2020
@driusan driusan merged commit f321d10 into aces:23.0-release Dec 23, 2020
@ridz1208 ridz1208 added this to the 23.0.3 milestone Feb 6, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants