cht-core icon indicating copy to clipboard operation
cht-core copied to clipboard

feat(#8461): add contact summary client side error log

Open jonathanbataire opened this issue 1 year ago • 11 comments

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 avatar Aug 11 '23 08:08 jonathanbataire

@jonathanbataire Is this ready for review?

garethbowen avatar Aug 30 '23 21:08 garethbowen

@jonathanbataire Is this ready for review?

@garethbowen this is ready for review

jonathanbataire avatar Sep 12 '23 06:09 jonathanbataire

@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.

garethbowen avatar Sep 12 '23 21:09 garethbowen

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Sep 13 '23 04:09 sonarqubecloud[bot]

@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 avatar Sep 19 '23 19:09 jkuester

@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 avatar Oct 18 '23 02:10 garethbowen

@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....

jkuester avatar Oct 23 '23 13:10 jkuester

@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?

garethbowen avatar Jan 25 '24 09:01 garethbowen

Apologies, this skipped my radar - distracted with the Nairobi trip last year. I'm going to take a look and comment here.

latin-panda avatar Jan 26 '24 11:01 latin-panda

thanks @latin-panda following up on this

jonathanbataire avatar Jan 30 '24 06:01 jonathanbataire

I've updated the PR title to match the format of feat(#8461): ......

latin-panda avatar Mar 26 '24 09:03 latin-panda

@jonathanbataire, the CI errors are weird, can you try downloading the latest from master branch? and maybe they get fixed

latin-panda avatar Jul 03 '24 04:07 latin-panda

@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

latin-panda avatar Jul 03 '24 05:07 latin-panda

@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 avatar Jul 03 '24 05:07 latin-panda

@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.

garethbowen avatar Jul 03 '24 05:07 garethbowen

I don't see any option to restart the sonar from my user (sonar link)

Screenshot 2024-07-03 at 12 57 28 PM

@jonathanbataire, you can try pushing a commit, like a space or something small to see if Sonar picks it up

latin-panda avatar Jul 03 '24 06:07 latin-panda

@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 avatar Jul 08 '24 13:07 garethbowen

@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 avatar Jul 08 '24 13:07 latin-panda

@latin-panda Yes I think that's right. Can you run sonar locally to see if this PR passes?

garethbowen avatar Jul 08 '24 16:07 garethbowen

: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...

jkuester avatar Jul 08 '24 19:07 jkuester

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 avatar Jul 08 '24 21:07 jkuester

@jkuester Thanks for confirming.

@jonathanbataire Thanks for your patience! This is now merged.

garethbowen avatar Jul 09 '24 04:07 garethbowen