BIDMCDigitalPsychiatry / LAMP-platform

The LAMP Platform (issues and documentation).
https://docs.lamp.digital/
Other
12 stars 10 forks source link

Audio Diary Activity Not Appearing in the Portal #723

Closed ertjlane closed 1 year ago

ertjlane commented 1 year ago

Describe the bug An audio diary is not appearing in portal, across multiple servers. This has been observed on multiple computer devices.

To Reproduce

  1. Import the attached file to a group
  2. Complete an audio diary (the attached activity) for a participant in that group.
  3. Go into the participants portal page and look at 'activities'.

Expected behavior A record of the audio diary's completion should appear as with other activities. Example of an activity record appearing in the portal can be seen below.

Screen Shot 2022-12-09 at 1 51 54 PM (2)

english-voice_v1.0_2022-05-28 (1).json.zip

falmeida-orangeloops commented 1 year ago

Here's what I've found so far:

falmeida-orangeloops commented 1 year ago

@ertjlane following what we were saying: I get that the reason why it's hidden could be that these kinds of activities don't involve visualizations and so they would look weird there. I also get that some kind of acknowledgement of the activity being completed is still a need. In case that is the reason, how would you like me to accomplish this? Maybe showing like just a check or something?

jtorous commented 1 year ago

I agree just a check with a date would be fine,

Thanks, John

On Wed, Dec 14, 2022 at 7:47 AM Facundo Almeida @.***> wrote:

@ertjlane https://github.com/ertjlane following what we were saying: I get that the reason why it's hidden could be that these kinds of activities don't involve visualizations and so they would look weird there. I also get that some kind of acknowledgement of the activity being completed is still a need. In case that is the reason, how would you like me to accomplish this? Maybe showing like just a check or something?

— Reply to this email directly, view it on GitHub https://github.com/BIDMCDigitalPsychiatry/LAMP-platform/issues/723#issuecomment-1351291907, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMFRATDJRUI6BBYJKOXTPTWNG6VVANCNFSM6AAAAAASZU5FHU . You are receiving this because you are subscribed to this thread.Message ID: @.*** com>

ertjlane commented 1 year ago

I've touched base with Aditya & ZCO regarding this. The audio journals were removed from the dashboard because loading the actual audio data (stored as a binary file) resulted in significant dashboard slowdown. There are two potential workarounds to this, both of which involve masking the binary data so the journal recordings themselves are not actually shown, but the activity completion is recorded (credit to Aditya for these solutions).

First, directly beneath this line, one can specify filters that filter out binary data. Secondly, this can be done by using .json data (document transforms). The difference between 1 & 2 being that in the former option the data is never sent from the server, and the latter the data is sent, but filtered out from the dashboard so it never has to travel over the internet. The benefit of using a .json structure is that it is is already implemented as a formal API and can be used to make a 3-4 line patch to the dashboard. The drawback is that there will still be some slowdown, but far less than if the data were loaded into the dashboard. So it is a question of how quickly the former can be implemented. If it is in a very short timeframe (several days - the next week), then I suggest we move to the former. Let me know if all of this makes sense.

I also think in terms of visual rather than a checkmark, it may be useful to have a block with the number of diaries submitted and the date of the last, as shown in the screenshot below (it's the same format we use for journals)!

Screen Shot 2022-12-13 at 11 40 54 AM (2)
falmeida-orangeloops commented 1 year ago

That seems reasonable! Will do. Thank you for the thorough background, much appreciated!

ertjlane commented 1 year ago

Any updates on this?

falmeida-orangeloops commented 1 year ago

Not yet, I have been working on BIDMCDigitalPsychiatry/LAMP-dashboard#705 in the last days

falmeida-orangeloops commented 1 year ago

@ertjlane A couple of updates:

As to how to mask out binary audio data, both options are pretty simple. The first one is the most straightforward and involves less server overhead. The second one is more elegant because it offers greater flexibility in the sense that no changes need to be made to the server, which means that the server would not need to be updated for these changes to take effect. This also means that if these changes need for some reason to be rolled back in the future, it would be simpler as it would only involve dashboard changes. If this is unlikely to happen, I think option 1 it will do. I can have that done in the course of this week.

In second place, I tried bringing back what was done UI-wise for audio journal thumbnails and screens. If I uncomment the existing code, here is what I see:

Screen Shot 2022-12-20 at 09 46 50 Screen Shot 2022-12-20 at 09 46 13

This UI seems pretty similar to what you describe. If this is the case, nothing extra needs to be done about the UI. Anyway, let me know whether it looks okay for you and also of any changes you'd like to make.

One last question: if I'm understanding you correctly, since audio data would not be downloaded from the server, there would be no point in having those audio players in the screen from the second screenshot. What should we do about this? Do you think just removing them but leaving the list of dates ("this month"/"all") and the calendar on the side would be okay?

ertjlane commented 1 year ago

@falmeida-orangeloops Thanks for the speedy response! To your first question, I think quick and dirty (option 1) will do. To your second question, the UI looks good! I don't think it makes sense to have the audio players, since I don't think they would have data to play? I think leaving the list of dates makes perfect sense.

falmeida-orangeloops commented 1 year ago

Cool! I'll be working on this next then.

falmeida-orangeloops commented 1 year ago

I've been looking for a way to conditionally exclude ActivityEvent's url field from what is returned by the DB if it contains binary audio data. This would be optimal performance-wise since that audio data would not even be downloaded from the DB by the server. Unfortunately there doesn't seem to be a way (that I know of) to conditionally exclude a field like this (I tried using projections, transformations and $function from the MongoDB API with no success).

So to avoid further delaying this I chose to allow the server to just keep querying that audio field from the DB without returning it to the dashboard. This is sub-optimal from the server's point of view, but will do for our purposes (i.e. avoiding the dashboard to download that audio data).

Server PR: BIDMCDigitalPsychiatry/LAMP-server#217 Dashboard PR: BIDMCDigitalPsychiatry/LAMP-dashboard#707

ertjlane commented 1 year ago

Sounds good! Do we expect this to be operating in dashboard staging?

falmeida-orangeloops commented 1 year ago

Should be, yes

ertjlane commented 1 year ago

Just checked, looks good on my end as well!

ertjlane commented 1 year ago

For some reason it appears that the audio diary visual isn't appearing in the portal. It is visible in staging, but not in production for some reason. The lamp-dashboard release including this was made over two weeks ago, so some sort of error has occurred.

falmeida-orangeloops commented 1 year ago

I double-checked the relevant PR (BIDMCDigitalPsychiatry/LAMP-dashboard#707) again, and nothing in it does things differently for production environment than for development environment (remember I basically uncommented some code). This most probably means the latest dashboard version is not yet deployed to production. Could you confirm this has been done?

ertjlane commented 1 year ago

The dashboard version including the update for this issue was pushed to production on the 4th, seen in the screenshot below.

Screen Shot 2023-01-24 at 10 55 37 AM
ertjlane commented 1 year ago

I'm pulling in ZCO for this issue, as orangeloops is now largely focused on Oauth. @ZCOEngineer could you please take a look at this in the next 24 hours and report back an assessment and/or timeline?

ZCOEngineer commented 1 year ago

@ertjlane we shall check and comment

sarithapillai8 commented 1 year ago

@ertjlane We don't understand why that change is not reflected in production. When checking dates and commits, the updates should be there in production. But we don't know how you created the release. Do you want us to change this and make a release up to that particular commit?

ertjlane commented 1 year ago

Thanks for looking into this. I'm a bit confused by your points, I believe the release is here: https://github.com/BIDMCDigitalPsychiatry/LAMP-dashboard/releases/tag/2023.01.04. Created the same way I usually make a release, by executing the 'release' button in the corresponding repository. Unless you mean something else?

sarithapillai8 commented 1 year ago

@ertjlane Yes the release shows the commits including this portal changes and it shows success in actions tab. Not sure what happened. We don’t have the access to the other details to check whether the pull to the server was actually a success. So we can try a new release pointing to that particular tag again, if possible. Note: Master branch having some incomplete items like 2FA, so we cannot try a new release in the same method we follow.

ertjlane commented 1 year ago

Understood, l think that makes sense!

sarithapillai8 commented 1 year ago

@ertjlane LAMP-server was not released for the audio diary changes. Only dashboard was released. TypeRepository changes are there in LAMP-server repo for 2FA changes. So please confirm if they are ready for production and after that could you release that repo too?

ertjlane commented 1 year ago

Sounds good, and yes once you confirm that I'll make the release.

sarithapillai8 commented 1 year ago

@carlan1 released LAMP-server and still, the audio diary is not listed in production. but it is listed in dashboard-staging with api-lamp.digital server login. @ertjlane Could you check with the team if anything missing in the production environment?

ertjlane commented 1 year ago

@sarithapillai8 Thanks for updating on this. The audio diaries are still missing in production.

sarithapillai8 commented 1 year ago

@ertjlane I am not sure about this. dashboard code build seems the same in staging and production when looking through the code. Could you please check with the team involved in this change to make sure anything else is missing from the production update?

ertjlane commented 1 year ago

@sarithapillai8 Checking on this, I'll circle around shortly.

ertjlane commented 1 year ago

@sarithapillai8 Previous investigations into this believed that it may be an issue with the release tag. Releases with leading zeroes are invalid, and the release (and subsequent re-release) had such. I will investigate further how to address this and ping you if anything further is needed from your end.

ertjlane commented 1 year ago

@sarithapillai8 I have conducted a release with the proper formatting for the dashboard, and the issue is still appearing. Is there any possibility that if the original code was released in an invalid release it wouldn't show up with a new (proper) release for the dashboard?

sarithapillai8 commented 1 year ago

We never faced an issue like this. Are we missing any other change? Other than LAMP-dashboard and LAMP-server repo?

ZCOEngineer commented 1 year ago

Based on todays discussion @ertjlane will check more on this.

ertjlane commented 1 year ago

@avaidyam Hello Aditya. In previous discussions with orangeloops, you noted that previous updates had failed due to leading '0's in the release tag, ex: 2023.01.04. As a solution, last week I made a new release in lamp-dashboard, the repo that appears to be pertinent. However, the audio diaries still appear not be showing up in the test accounts I've utilized in the portal. Do you have any additional insight on this? I've tried with multiple user accounts, and after clearing my web browser cache.

avaidyam commented 1 year ago

It doesn't seem like the release you made worked since the production dashboard is still on v2226 (2022.12.15).

@carlan1 Can you double check the stack file in the console and ensure the release specified is :2023?

@ertjlane Can you ensure all the repositories in the stack file with :2022 releases have at least one release made in 2023? If not, please go to their repositories and make a new release even if there are no changes. Then, you or @carlan1 please update the stack file to change ALL :2022 to :2023 releases.

avaidyam commented 1 year ago

@ertjlane I've just manually updated the service. Please check to see if the issue is fixed now.

ertjlane commented 1 year ago

Carsten and I have checked the stack and made the appropriate adjustments. This issue has been resolved, the audio diaries are now appearing in the production dashboard.