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

Set default value for storage limit slider #11191

Merged
merged 6 commits into from
Sep 25, 2023

Conversation

nikkuAg
Copy link
Contributor

@nikkuAg nikkuAg commented Sep 4, 2023

Summary

I've implemented a functionality that sets the limitForAutodownload to 80% of the available freeSpace if the checkbox is not selected; otherwise, it retrieves the value from the API.

image

device-settings-google-chrome-2023-09-05-00-00-56_OOUXovPp.mp4

References

#11176

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) SIZE: small labels Sep 4, 2023
@MisRob MisRob requested review from MisRob and pcenov September 5, 2023 11:55
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Hi @nikkuAg and @MisRob, this is working mostly correctly now, however in my Ubuntu VM I am seeing the value in GB as 60.800000000000004 which is then displayed as 60GB if I save the changes and refresh the page. I think this value should be always rounded and in this case it should be rounded to 60.8 GB

2023-09-05_16-42-54.mp4

@nikkuAg
Copy link
Contributor Author

nikkuAg commented Sep 5, 2023

I will look into it

@nikkuAg
Copy link
Contributor Author

nikkuAg commented Sep 6, 2023

@pcenov I have solved the bug

@pcenov pcenov self-requested a review September 8, 2023 08:14
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Thank you @nikkuAg I confirm that this is fixed now!

@@ -560,7 +560,7 @@
},
created() {
this.setDeviceURLs();
this.setFreeSpace();
if (this.freeSpace === 0) this.setFreeSpace();
Copy link
Member

Choose a reason for hiding this comment

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

I can imagine a situation where the freeSpace value is actually 0 - i.e. when there is no free space on the device. Perhaps it would be better if this were initialized as null rather than 0 to be able to do a check on whether it has been set or not (would have to make sure anywhere it was being used was being guarded against the non-numeric value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the changes

@nikkuAg
Copy link
Contributor Author

nikkuAg commented Sep 19, 2023

@rtibbles @pcenov
The yarn run devserver command is giving an error suddenly: uncaught reference error: kolibricoreappglobal is not defined.

@MisRob
Copy link
Member

MisRob commented Sep 21, 2023

@nikkuAg can you try to run make clean before running the dev server?

@nikkuAg
Copy link
Contributor Author

nikkuAg commented Sep 21, 2023

@MisRob
After running the command make clean and then yarn run devserver I am getting the following error

image

@MisRob
Copy link
Member

MisRob commented Sep 21, 2023

@nikkuAg Is your virtual environment activate? Could you reinstall Python packages? If that doesn't help, I would ask you for all logs that you can send us.

@MisRob
Copy link
Member

MisRob commented Sep 21, 2023

@nikkuAg Ah now I've remembered that after make clean you will need to run pip install -e . (don't forget the dot). So try this at first please.

@nikkuAg
Copy link
Contributor Author

nikkuAg commented Sep 21, 2023

@MisRob Thank you for the help, it is working now.

@MisRob MisRob requested a review from rtibbles September 22, 2023 13:42
@rtibbles rtibbles merged commit 6de19e1 into learningequality:release-v0.16.x Sep 25, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants