cht-core
cht-core copied to clipboard
feat(#8461): add contact summary client side error log
Description
medic/cht-core#8461
Code review checklist
- [ ] Readable: Concise, well named, follows the style guide, documented if necessary.
- [ ] Documented: Configuration and user documentation on cht-docs
- [ ] Tested: Unit and/or e2e where appropriate
- [ ] Internationalised: All user facing text
- [ ] Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.
Compose URLs
If Build CI hasn't passed, these may 404:
- CHT_CORE_COMPOSE_URL
- COUCH_SINGLE_COMPOSE_URL
- COUCH_CLUSTER_COMPOSE_URL
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.
@jonathanbataire Is this ready for review?
@jonathanbataire Is this ready for review?
@garethbowen this is ready for review
@jkuester As you reviewed the work this builds on, would you mind reviewing please?
The builds that this is waiting on have been renamed so it's not gone green. @jonathanbataire can either rebase, or I can use my admin perms to bypass the check.
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells
No Coverage information
0.0% Duplication
@tatilepizs this is in good shape. The e2e test is already done! Can you have a look and let us know if you have any suggestions/concerns?
@jkuester It looks like you're happy with this change but GH still thinks you've requested changes. If you're happy, can you please hit the big merge button, and close the issue?
@garethbowen I am good with the overall behavior of the code, but I am still a bit concerned about unintended consequences coming from the change to the error flow. Was hoping to hear from @latin-panda (or another Angular expert) if there was a better way to do error propagation when subscribed to a store
....
@latin-panda Josh hoped you could have a look here and see if this is the right way to solve this issue so I've tagged you for review - hope that's ok?
Apologies, this skipped my radar - distracted with the Nairobi trip last year. I'm going to take a look and comment here.
thanks @latin-panda following up on this
I've updated the PR title to match the format of feat(#8461): ......
@jonathanbataire, the CI errors are weird, can you try downloading the latest from master
branch? and maybe they get fixed
@garethbowen have you seen this error before?
Error: the string "Error response from daemon: manifest for medicmobile/cht-haproxy:4.10.0-8039-contactsummary-log.9772146005 not found: manifest unknown: manifest unknown\n" was thrown, throw an Error :)
The PR changes don't seem related to this, and the branch has been updated with the latest from master
.
It's consistently failing, I don't see other cht core dev with the same issue
@garethbowen I see this feat(#9227): add validation function for luhn algorithm
from ChinHairSaintClair has the same problem, so maybe it's because it's coming from a contributor
@latin-panda I think you're right. Please raise an issue to fix the k3d tests for external contributors.
For now, those builds aren't required so we can ignore them. There are two other problems - one test (probably flaky) and SonarCloud isn't responding.
I don't see any option to restart the sonar from my user (sonar link)
@jonathanbataire, you can try pushing a commit, like a space or something small to see if Sonar picks it up
@latin-panda Let's get this merged. I know the k3d builds are failing because of a known issue. Do you know what the issue is with sonar? I wonder if it's also not working for external contributors. Can you investigate and let me know?
If everything's fine then I can merge it bypassing branch protections - just let me know!
@garethbowen It seems like my user in Sonar UI is limited (sonar link). I don't see any action I can click to help passing this PR. It's stuck in an old commit and not receiving the new ones.
Another contributor seems to have the problem add validation function for luhn algorithm
I cannot even find the PR in sonar. Perhaps something to do in project config of Sonar for Anonymous users.
@latin-panda Yes I think that's right. Can you run sonar locally to see if this PR passes?
:thinking: I don't think we are having Sonar problems for all contributor PRs, since I think it was working okay for the Draw Widget PR. I do notice that the last time Sonar analyzed this PR was on the last merge-commit from master
. I wonder if something has gotten funky with how Sonar is seeing the commit graph here. My suggestion would be to try and update the branch with a fresh merge-commit from master
and see if Sonar picks things back up from there...
Hmm, rats. That does not seem to have helped. :sweat: I also do not see any way to manually trigger the check from the SonarCloud side of things.
One complicating factor is that it looks like this PR was opened before we actually enabled Sonar on this repo. That may be one reason why this is going so weird.... Either way, @garethbowen, it should be safe to just override the check and merge this PR. I created a new branch/PR with the same HEAD commit and it passed the Sonar checks fine. So, no concern here.
@jkuester Thanks for confirming.
@jonathanbataire Thanks for your patience! This is now merged.