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

Informal facilities get 'Create an Account' button visible by default #7553

Merged
merged 1 commit into from Oct 12, 2020
Merged

Informal facilities get 'Create an Account' button visible by default #7553

merged 1 commit into from Oct 12, 2020

Conversation

AdamStasiw
Copy link
Contributor

@AdamStasiw AdamStasiw commented Oct 9, 2020

Summary

This is to fix #7505, such that facilities created with the "informal" settings will allow the creation of new learners by default. This way, when doing initial setup with the "Quick start" approach, the admin doesn't need to activate this setting manually, and users will be able to create accounts by default.

Steps Taken:

  1. Initialize the app with a fresh database
  2. Create a new facility using "Quick start"
  3. Logout of the resulting admin account
  4. Observe the log-in landing page

Before:
Screen Shot 2020-10-09 at 5 51 11 PM

After:
Screen Shot 2020-10-09 at 6 16 30 PM

Clicking on "Create an Account" proceeds to the user creation page:
Screen Shot 2020-10-09 at 6 16 50 PM

I also modified the test_personal_setup_defaults test to account for this new default value. Running pytest kolibri/core/device/test/test_api.py completed successfully.

Reviewer guidance

Since I'm brand new to the codebase (and this is actually my very first open source contribution ever!), I'm not sure if I should run other tests or if the CR tool runs them for me. To be safe, I ran tox and pytest to try running everything. I got mostly all successes, but then a collection of failures/errors before the tests started to hang on discovery/test/test_connection_check.py - though I tried again on the base develop branch without my changes, and got the exact same errors:

kolibri/core/content/test/test_import_export.py .........FFFFFF.F.FFFFFFEEEEEEEEEEE                                                                                                                                                  [ 50%]
kolibri/core/content/test/test_importability.py FF                                                                                                                                                                                   [ 50%]
kolibri/core/content/test/test_movedirectory.py FFFFFFF                                                                                                                                                                              [ 51%]
kolibri/core/content/test/test_redirectcontent.py EEEEEEEEEEEE                                                                                                                                                                       [ 51%]
kolibri/core/content/test/test_sqlalchemy_bridge.py ...........................                                                                                                                                                      [ 53%]
kolibri/core/content/test/test_upgrade.py FFFFFFFFFF                                                                                                                                                                                 [ 54%]
kolibri/core/content/test/test_zipcontent.py FFFFFFFFFFFFFFFFFFFFFFFFFF

Not sure if this is something I'm doing wrong in my local testing?

References

See #7505 for the original issue / motivation for the change


Contributor Checklist

PR process:

  • PR has the correct target branch and milestone (I don't think I have the ability to add a milestone?)
  • PR has 'needs review' or 'work-in-progress' label (Similarly, I couldn't find how to add a label to this Pull Request)
  • 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

Testing:

  • 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

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

Adding myself to the authors.md file - my very first open source contribution!
@AdamStasiw AdamStasiw marked this pull request as ready for review October 9, 2020 23:17
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #7553 into develop will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted Files Coverage Δ
kolibri/core/tasks/utils.py 87.03% <0.00%> (-5.56%) ⬇️

@jonboiser jonboiser added this to the 0.14.4 milestone Oct 12, 2020
@jonboiser
Copy link
Contributor

Hi @AdamStasiw , thanks for this patch!

When running your tests using pytest or make test, does the suite pass? I generally don't expect all the environments in tox to work locally, and just worry if it passes on Travis (which it did here). So if the tests mostly pass with a simple make test command, that's good enough for me.

Also, this is a bit of internal process that isn't documented (yet) for community contributors like yourself, but issues with the 0.14.4 milestone need to be based on the release-v0.14.x branch. Issues with the 0.15.0 milestone would be based on the develop branch. What I'll do is merge this one to develop and cherry-pick it to the 0.14 branch so it gets into the next release!

@jonboiser jonboiser added changelog Important user-facing changes TODO: needs gherkin update Add to our manual integration tests labels Oct 12, 2020
Copy link
Contributor

@jonboiser jonboiser left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@jonboiser jonboiser merged commit bf91a09 into learningequality:develop Oct 12, 2020
@jonboiser
Copy link
Contributor

jonboiser commented Oct 12, 2020

@AdamStasiw If you're interested in a related issue, check out #7334 which is a fix to the Vue frontend code.

Basically, the problem is that when you click the "reset to default" button in the Facility Settings page, the "defaults" are not the ones chosen during setup. For example, if I set up my device using the quick setup (i.e. with the "personal/informal" preset) and then click the "reset to default" button later, the "shown download button with resources" setting gets unchecked).

@AdamStasiw
Copy link
Contributor Author

Ah okay, awesome - thank you!

And I'm much more of a backend/Python developer than a frontend one, but I can give #7334 a look!

@AdamStasiw AdamStasiw deleted the adam/7505/personal_learners_signup branch October 12, 2020 18:49
@jonboiser
Copy link
Contributor

Actually, the fix to #7334 might be better handled on the backend. I'll add some notes to the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Important user-facing changes TODO: needs gherkin update Add to our manual integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Informal (aka "personal use") facilities should have "learners can sign up" default to true.
2 participants