Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Analyses results view #2016

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Analyses results view #2016

wants to merge 31 commits into from

Conversation

hudson-newey
Copy link
Member

@hudson-newey hudson-newey commented Dec 9, 2022

Default Analyses results view

Downloading the default analyses from audio recordings

Changes

  • Created analysis download row components
  • Creates new routes for analyses downloads
  • Creates analysis results component

Problems

Download functionality for directories in the form of ZIP files needs to be completed as part of another ticket

Issues

Fixes: #2011

Visual Changes

image

Final Checklist

  • Assign reviewers if you have permission
  • Assign labels if you have permission
  • Link issues related to PR
  • Ensure project linter is not producing any warnings (npm run lint)
  • Ensure build is passing on all browsers (npm run test:all)
  • Ensure CI build is passing
  • Ensure docker container is passing (docs)

@hudson-newey
Copy link
Member Author

hudson-newey commented Dec 9, 2022

Todo:

  • Unit tests for opening and closing a folder in Analysis Results view
  • Loading rows through be done asynchronously using an observable code (at the moment it's rendering opening a folder by code, this is obviously not ideal)
  • Method for converting large bytes to readable format (KB, MB, GB, etc...) needs to either a. find a library or b. be refactored code
  • Link view to real data, (instead of mock / predetermined data) code
  • Bind analyses-results component to AnalysisResults model
  • Refactor & clean up code quality
  • Investigate why faker.js is causing bundle to be so large (causing CI to fail)
  • Change getItems() method in analyses-results.component.ts to an async/await method. This should fix this race conditions & unit tests

@hudson-newey hudson-newey added enhancement New feature or request work in progress Pull request that is currently a WIP ui feature A new feature/page to add to the project triage:high High priority issue or pull request missing tests Pull request missing unit testing. labels Dec 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Unit Test Results

         6 files           6 suites   16m 4s ⏱️
17 616 tests 17 052 ✔️ 564 💤 0
17 724 runs  17 160 ✔️ 564 💤 0

Results for commit a00e908.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 12, 2022

Size Change: +10.1 kB (0%)

Total Size: 2.68 MB

Filename Size Change
dist/workbench-client/browser/index.html 3.85 kB +17 B (0%)
dist/workbench-client/browser/main.****************.js 849 kB +4.15 kB (0%)
dist/workbench-client/browser/styles.****************.css 37.5 kB +17 B (0%)
dist/workbench-client/server/main.js 1.78 MB +5.87 kB (0%)
ℹ️ View Unchanged
Filename Size
dist/workbench-client/browser/assets/environment.json 555 B
dist/workbench-client/browser/manifest.json 150 B
dist/workbench-client/browser/polyfills.****************.js 12.5 kB
dist/workbench-client/browser/runtime.****************.js 648 B

compressed-size-action

@hudson-newey
Copy link
Member Author

hudson-newey commented Dec 15, 2022

I'm currently observing a race condition.
It is originating from this getItems() method in analyses-results.component.ts. To fix this, I am going to be changing getItems() to an async/await method, and am going to change the api.list() to api.show(). This is the last remaining task, everything else should be all done and ready for review

@hudson-newey hudson-newey marked this pull request as ready for review December 15, 2022 03:38
Copy link
Member

@atruskie atruskie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important you discuss with me API deficiencies before you code around them. Maybe it is something that can be fixed, maybe your understanding is incorrect.

Why are you using promises when observables are supported by the async pipe as well?

Okay looks promising but I'd say another solid pass is needed.

Comment on lines 8 to 13
<span class="whitespace-indentation" *ngFor="let i of indentation()">
<baw-directory-whitespace
[isFolder]="isFolder"
[open]="open"
></baw-directory-whitespace>
</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about our custom directive accepts an indentation and just uses a single element with variable width?

src/app/models/AnalysisJobItemResult.ts Outdated Show resolved Hide resolved
src/app/models/AnalysisJobItemResult.ts Outdated Show resolved Hide resolved
src/app/models/AudioRecording.ts Show resolved Hide resolved
@hudson-newey
Copy link
Member Author

hudson-newey commented Dec 16, 2022

I know this is a big PR, so thanks for the review. I'm going through the requested changes now and will get back to you asap

@hudson-newey hudson-newey removed the missing tests Pull request missing unit testing. label Dec 20, 2022
@hudson-newey hudson-newey removed the work in progress Pull request that is currently a WIP label Dec 23, 2022
@hudson-newey hudson-newey requested a review from atruskie January 2, 2023 13:59
@hudson-newey hudson-newey self-assigned this Jan 3, 2023
…lyses-results.component.ts

Co-authored-by: Anthony Truskinger <[email protected]>
@hudson-newey hudson-newey added the work in progress Pull request that is currently a WIP label Jan 10, 2023
@hudson-newey
Copy link
Member Author

hudson-newey commented Jan 10, 2023

The code currently pushed is not ideal & is of bad quality, but has the requirements listed above implemented (sub 1).

I am going to be cleaning up the code quality before submitting it for review, but I wanted to push it to avoid losing my work due to a hard drive failure, etc... and create a point of reference for a very rough MVP.

Personal Review TODO list

  • Fix indentation method
  • Update analyses-results.component.spec.ts test
  • Update analysis-download-row.component.spec.ts tests
  • Update AudioRecording.spec.ts tests
  • Add unit tests for file Array.prototype.sort() method
  • Using filter & map methods means the this.rows array is a new instance every change. Therefore the observable isn't being updated. I need to find a fix for this
  • Remove root path from analysis-results.component.ts
  • Fix system routes in menu items
  • Change typescript access modifiers to better reflect usage
  • Add code comments
  • Add @BawBytes decorator to AnalysisJobItemResult model
  • Write JsDoc comments
  • Consolidate duplicate code in analysis-download-row.component.ts
  • Fix download button for folders of more than 2 levels of depth
  • Add error notification if there are no analysis job item results
  • Fix eslint errors
  • Move system analysis job to menu items instead of routes
  • Fix code comment in analysis-download-row.component.spec.ts
  • Assert that buttons next to files are not disabled
  • Use analysis-download-row.component.ts view model in tests
  • Remove API root references from analysis-download-row.component.ts (it shouldn't have any knowledge of routes)
  • Remove reminence of using px in css and replace with em / rem
  • Change even checkered background colour to use css variables defined in the global style
  • Change analysis-results.component.spec.ts default audio recording not to have a specified ID attribute
  • See if setup() in analysis-results.component.spec.ts needs { detectChanges: false } (it does)
  • In getDownloadButton in analysis-results.component.spec.ts I should use By.css() instead of by index (By.css only works on jasmine fixtures, not HTML elements, this is NA)
  • Remove re-instantiating AnalysisJobItemResult model on ResultNode (not needed because the models are incomplete)
  • Download button tests sometimes fail randomly
  • Fix view on mobile

@hudson-newey hudson-newey requested a review from atruskie January 12, 2023 01:47
@hudson-newey hudson-newey removed the work in progress Pull request that is currently a WIP label Jan 12, 2023
@atruskie atruskie added the blocking/blocked Pull requests or issues that are blocked by something label Feb 27, 2023
@atruskie
Copy link
Member

I'm not going to merge this until associated changes with the API are finalized. https://github.com/QutEcoacoustics/baw-server/tree/pbs-analysis

@hudson-newey hudson-newey changed the title Default Analyses results view Analyses results view Apr 27, 2023
@hudson-newey hudson-newey added the work in progress Pull request that is currently a WIP label Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking/blocked Pull requests or issues that are blocked by something enhancement New feature or request triage:high High priority issue or pull request ui feature A new feature/page to add to the project work in progress Pull request that is currently a WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start to sketch out a UI for downloading the results
2 participants