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

pr/7283 setting doesn't get disabled in valid state #7446

Merged
merged 5 commits into from Aug 17, 2020

Conversation

mdctleo
Copy link
Contributor

@mdctleo mdctleo commented Aug 6, 2020

Summary

setting doesn't get disabled in valid state #7283

Aug-06-2020 01-03-15

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

@mdctleo mdctleo changed the title checkboxes fix PR/7283 Aug 6, 2020
@mdctleo mdctleo changed the title PR/7283 pr/7283 Aug 6, 2020
@mdctleo mdctleo changed the title pr/7283 pr/7283 setting doesn't get disabled in valid state Aug 6, 2020
@mdctleo
Copy link
Contributor Author

mdctleo commented Aug 6, 2020

I can't seem to change label to work-in-progress for this pull request, perhaps due to permission

@indirectlylit indirectlylit added the work-in-progress Not ready for review label Aug 6, 2020
@indirectlylit
Copy link
Contributor

@mdctleo I've added the work-in-progress label

thanks for your contribution, and let us know when it's ready for review

@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #7446 into develop will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted Files Coverage Δ
...lity/assets/src/views/FacilityConfigPage/index.vue 60.00% <0.00%> (-4.71%) ⬇️
...bri/plugins/learn/assets/src/views/ChannelCard.vue 44.44% <0.00%> (-12.70%) ⬇️
...ibri/plugins/learn/assets/src/views/TopicsPage.vue 25.00% <0.00%> (-2.59%) ⬇️
kolibri/core/lessons/serializers.py 73.40% <0.00%> (-2.56%) ⬇️
...ach/assets/src/modules/classSummary/dataHelpers.js 67.21% <0.00%> (-1.13%) ⬇️
kolibri/core/content/api.py 88.86% <0.00%> (-0.70%) ⬇️
kolibri/utils/server.py 37.02% <0.00%> (-0.13%) ⬇️
...libri/core/assets/src/api-resources/contentNode.js 0.00% <0.00%> (ø)
...plugins/learn/assets/src/views/RecommendedPage.vue 0.00% <0.00%> (ø)
...gins/learn/assets/src/modules/recommended/index.js 0.00% <0.00%> (ø)
... and 18 more

@mdctleo
Copy link
Contributor Author

mdctleo commented Aug 7, 2020

@indirectlylit I noticed that the original issue is tagged with frontend:dev, is backend validation necessary for this change? Thanks!

@mdctleo
Copy link
Contributor Author

mdctleo commented Aug 12, 2020

@indirectlylit bumping this for review

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

Thanks @mdctleo

By the way if you aren't in there, feel free to add yourself to https://github.com/learningequality/kolibri/blob/release-v0.14.x/AUTHORS.md

I noticed that the original issue is tagged with frontend:dev, is backend validation necessary for this change?

The tags are definitely not strict.

If you'd like to adding back-end validation (and maybe even a unit test) in addition, that would be great! It's always better to enforce consistency in the API when possible. However the system should also not crash if inconsistent values somehow got set in an older API or DB.

@jonboiser jonboiser added this to the 0.15.0 milestone Aug 17, 2020
@jonboiser jonboiser added changelog Important user-facing changes and removed work-in-progress Not ready for review labels Aug 17, 2020
@jonboiser jonboiser merged commit 3c707d8 into learningequality:develop Aug 17, 2020
jonboiser added a commit that referenced this pull request Sep 28, 2020
Co-authored-by: mdctleo <mdctleo@users.noreply.github.com>
@jonboiser jonboiser modified the milestones: 0.15.0, 0.14.4 Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Important user-facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants