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

Fixed settings pages #1952

Merged
merged 29 commits into from
Jun 18, 2020
Merged

Conversation

jayoshih
Copy link
Contributor

Description

Finished settings page work

Implementation Notes (optional)

Does this introduce any tech-debt items?

  • There are some tests under storage.spec.js and deleteAccountForm.spec.js that are commented out due to KTheme attributes that aren't available. However, when I tried to import it under jest_config/setup, it threw an error
  • Need to enable validation on all fields on submit
  • Need to add policy modal

Checklist

  • Is the code clean and well-commented?
  • Has the docs label been added if this introduces a change that needs to be updated in the user docs?
  • Has the CHANGELOG label been added to this pull request? Items with this label will be added to the CHANGELOG at a later time
  • Are there tests for this change?
  • Are all user-facing strings translated properly (if applicable)?
  • Has the notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)?
  • Are all UI components LTR and RTL compliant (if applicable)?

@jayoshih jayoshih requested a review from nucleogenesis June 12, 2020 23:55
@jayoshih jayoshih added the qa-ready Create a demo server for this pull request label Jun 15, 2020
@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

Merging #1952 into vue-refactor will increase coverage by 3.23%.
The diff coverage is 78.91%.

Impacted file tree graph

@@               Coverage Diff                @@
##           vue-refactor    #1952      +/-   ##
================================================
+ Coverage         75.63%   78.87%   +3.23%     
================================================
  Files               300      274      -26     
  Lines             14264    13659     -605     
================================================
- Hits              10789    10773      -16     
+ Misses             3475     2886     -589     
Impacted Files Coverage Δ
contentcuration/contentcuration/tasks.py 72.64% <0.00%> (ø)
contentcuration/contentcuration/urls.py 91.79% <ø> (ø)
contentcuration/contentcuration/views/settings.py 74.54% <57.14%> (+28.45%) ⬆️
contentcuration/contentcuration/serializers.py 68.58% <76.47%> (-0.42%) ⬇️
contentcuration/contentcuration/models.py 87.14% <80.00%> (-0.21%) ⬇️
contentcuration/contentcuration/forms.py 67.14% <88.33%> (+14.32%) ⬆️
...entcuration/contentcuration/tests/test_settings.py 95.83% <95.83%> (ø)
...entcuration/contentcuration/tests/test_db_stats.py 100.00% <100.00%> (ø)
contentcuration/contentcuration/tests/test_user.py 100.00% <100.00%> (ø)
...n/contentcuration/templatetags/translation_tags.py 72.72% <0.00%> (-3.04%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af765d7...805ae69. Read the comment docs.

@kollivier
Copy link
Contributor

I reviewed the user experience and things seem to work quite well there! Did not do a code review since I'm not familiar with the frontend internals.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

The only issue I found is that the name form isn't saving. Everything else LGTM

Comment on lines +76 to +77
// eslint-disable-next-line kolibri/vue-no-unused-methods
onSubmit(formData) {
Copy link
Member

Choose a reason for hiding this comment

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

This form is not saving I think this might be related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, it's something to do with vue responsivity. Just updated to use this.$set to manage form data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, should be fixed now

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Read through the code.

One suggestion to avoid doing N-queries in a for loop - nothing else jumped out at me.

@jayoshih jayoshih mentioned this pull request Jun 18, 2020
10 tasks
@rtibbles rtibbles merged commit 794737d into learningequality:vue-refactor Jun 18, 2020
@jayoshih jayoshih deleted the vue-refactor branch August 24, 2020 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qa-ready Create a demo server for this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants