Cht-core: Move various scope properties into redux store

Created on 29 Mar 2019  路  14Comments  路  Source: medic/cht-core

As part of the ongoing changes to get ready to move to Angular 7, this issue is for moving various scope properties into the redux store.

AT: Parts of the app touched include:

  • All loading screens
  • App "version" on the about page
  • Error messages on analytics, contacts, messages, reports, tasks, and the edit user modal
  • Url to the admin app
  • The "facilities" filter on the reports page
  • The currently selected document on analytics, contacts, messages, reports, and tasks

This is effectively a refactor with no new features added.

PR: https://github.com/medic/medic/pull/5644

2 - Medium Performance

All 14 comments

The PR need some reviewed before we are able to merge @rmhowe .
https://github.com/medic/medic/pull/5644

Moving to 3.6.0 so it doesn't destabalise 3.5.0.

@ngaruko it's been reviewed but can't be merged until the 3.5.x branch has been cut. Has it been ATed or were you waiting on the review?

Seeing this error @rmhowe and page loading a wee slower that usual.
Did you want to have a look at this? Happened in Firefox
image
Also, the 'task' tab for admins normally shows an message along the line of Tasks are disabled for admin users. If you need to see tasks, login as a normal user.
image

But now it shows no tasks
image

I started to have a look at this but it's more serious than I hoped. Tasks is not working at all now - it always says "no tasks found" even on an offline user when there are some. This is caused by variables not being bound/passed correctly so the $ctrl.tasksDisabled and $ctrl.hasTasks are undefined.

Also, the other tabs are showing bugs saying "no x found" when there clearly are, eg:

People:

Screenshot from 2019-07-02 11-02-00

Reports:

Screenshot from 2019-07-02 11-02-11

@rmhowe Could you please have another look at this PR and fix these issues? Also, could please rebase against master and resolve the conflicts so the AT will be close to what will actually be merged? If you don't have time in the next couple of days, pleaselet me know and I'll have another look.

@garethbowen @ngaruko sorry got tripped up by Angular template syntax, too used to React. Should be fixed now.

@rmhowe Thanks Robbie! That was a fast turnaround.

There is another strange behavior observed here @rmhowe . On creating a new person, it takes a wee longer for the medic ID to be generated and in the meantime, if one deletes a contact (before the ID was generated), the UI shows that the the person has been deleted, but in actuality it is not.
See video attached.
delete-contact.mov.zip

@ngaruko I'm having trouble recreating the bug in my local environment. The video is great but were there some steps before the start of that video that led to the bug? (eg the creating a new person step) Would you be able to make a video that includes the whole process?

@rmhowe I think it is a really edge case scenario, not likely to happen in real life. Hard to reproduce even. But the steps were: create a new person, and delete it before the medic ID is generated. At first there was quite a delay and that's when it was not actually deleted, though there was a confirmation thereof in the UI. Now, I tried to replicate, and what I get is a error in the console and actually the delete button not doing anything.
image.

create-delete-contact.mov.zip

As I said it is really not likely to happen in production but I guess if we were to automate the UI, that could be and issue, but also it might indicate another flaw in the code. So, feel free to investigate and fix .

@rmhowe I realised the above is happening even on master, so not related to the refactoring. I will leave this and raise a separate issue if needed. However, there is a glitch in facility filter on report page:
On Master
image

on the branch
image

@ngaruko thanks for sorting that out. With respect to the facility filter I've pushed a fix to the branch, however I'm not sure what the expected behaviour is on master due to some peculiar code here as a result of https://github.com/medic/medic/issues/4423, it's only allowing one facility to be displayed at a time? Regardless only showing one at a time appears to be the intended behaviour so then I believe my branch is behaving as expected.

Thanks @rmhowe There surely seems to be something weird with that feature. I will close this issue and investigate further on that and eventually raise a different issue for it!

Merged!

Was this page helpful?
0 / 5 - 0 ratings

Related issues

estellecomment picture estellecomment  路  5Comments

garethbowen picture garethbowen  路  5Comments

alxndrsn picture alxndrsn  路  6Comments

n-orlowski picture n-orlowski  路  5Comments

ngaruko picture ngaruko  路  3Comments