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.

Sign up for GitHub

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

Jump to bottom

fix: 🍰 Checkboxes Not Missing Anymore On Delete User Account Page #3506

Merged
merged 16 commits into from Aug 5, 2020
Merged

fix: 🍰 Checkboxes Not Missing Anymore On Delete User Account Page #3506

merged 16 commits into from Aug 5, 2020

Conversation

ogerly
Copy link
Contributor

@ogerly ogerly commented Apr 26, 2020

🍰 Pullrequest

the checkboxes for saved contributions and comments of a user are displayed again at userDelete

Issues

@ogerly ogerly added the bug label Apr 26, 2020
@ogerly ogerly self-assigned this Apr 26, 2020
@ogerly ogerly added this to To Do in Human-Connection via automation Apr 26, 2020
@cypress
Copy link

cypress bot commented Apr 26, 2020 •
edited


Test summary

66 • 0 • 0 • 0

Run details
Project Human-Connection
Status Passed
Commit 17a296b
Started Aug 5, 2020 7:13 AM
Ended Aug 5, 2020 7:35 AM
Duration 21:31 💡
OS Linux Ubuntu Linux - 16.04
Browser Firefox 68

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@cypress
Copy link

cypress bot commented Apr 26, 2020 •
edited


Test summary

66 • 0 • 0 • 0

Run details
Project Human-Connection
Status Passed
Commit 5f19153 ℹ️
Started Aug 5, 2020 7:14 AM
Ended Aug 5, 2020 7:36 AM
Duration 22:09 💡
OS Linux Ubuntu Linux - 16.04
Browser Firefox 68

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@rbeer
Copy link
Contributor

rbeer commented Apr 26, 2020 •
edited

Am I correct in assuming that the problem was that the lengths were missing, so that these v-if never come true?

Human-Connection/webapp/components/DeleteData/DeleteData.vue

Line 12 in 9aacb22

<label v-if="currentUser.contributionsCount" class="checkbox">

If this is broken and still made it to master, test coverage on DeleteData is probably not sufficient.

mattwr18
mattwr18 requested changes Apr 26, 2020
View reviewed changes
Copy link
Member

@mattwr18 mattwr18 left a comment

Choose a reason for hiding this comment

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

I don't know how, but somehow it seems we didn't add cypress tests for the DeleteUser feature, oder?
would you mind adding them so that we can have a higher confidence we don't accidentally break this functionality in the future?

Mogge
Mogge previously requested changes Apr 27, 2020
View reviewed changes
Copy link
Member

@Mogge Mogge left a comment •
edited

Choose a reason for hiding this comment

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

Great that you have found the error. As far as I remember, this change was made to reduce requests to the database. Each count is a separate request. I would keep that as it was and create a new query that is used only on the delete user account page.

@ogerly
Copy link
Contributor Author

ogerly commented Apr 27, 2020

I have removed the usercount from the currentUser.
the usercounts are no longer queried on the deleteUser. the checkboxes are displayed as cecked by default.

FireShot Capture 037 - Human Connection - Human Connection - localhost

FireShot Capture 036 - Human Connection - Human Connection - localhost

@ogerly ogerly requested review from Mogge and mattwr18 April 27, 2020 11:59
ogerly added 2 commits April 29, 2020 10:04
@ogerly
Copy link
Contributor Author

ogerly commented Apr 29, 2020 •
edited

deleteUser on old state, currentUserCounts are retrieved additionally, I have added the
webapp/graphql/User.js creates a currentUserCountQuery().
but somehow this is doubled.
but it is now only present in the delete-data page.

i have excluded the admin to leave the ship. a superadmin would have to be here before. since he is not here. an admin must not be allowed to be deleted via usersettings.
FireShot Capture 046 - Human Connection - Human Connection - localhost

and i have found the counting variant for the i18n in vuex and it works fine ... ;)

FireShot Capture 048 - vuex-i18n - npm - www npmjs com

@ogerly ogerly closed this Apr 29, 2020
Human-Connection automation moved this from To Do to Done Apr 29, 2020
@ogerly
Copy link
Contributor Author

ogerly commented Apr 29, 2020

sorry

@ogerly ogerly reopened this Apr 29, 2020
Human-Connection automation moved this from Done to In progress Apr 29, 2020
mattwr18
mattwr18 previously requested changes Apr 30, 2020
View reviewed changes
Copy link
Member

@mattwr18 mattwr18 left a comment

Choose a reason for hiding this comment

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

I still would not feel comfortable merging this with no e2e(cypress) tests that make sure we don't break this important feature in the future. Please add tests, and re-request a review.

webapp/components/DeleteData/DeleteData.vue Outdated
@@ -1,5 +1,5 @@
<template>
<base-card class="delete-data">
<base-card v-if="currentUser.role !== 'admin'" class="delete-data">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have agreed on this, therefore I would open an issue and we can discuss it, not just make this decision alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so i changed it

@ogerly ogerly requested a review from mattwr18 April 30, 2020 14:02
@ogerly ogerly dismissed stale reviews from mattwr18 and Mogge May 25, 2020 09:39

changes done

Tirokk
Tirokk requested changes May 27, 2020
View reviewed changes
Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

Hey, great job @ogerly 💫🐛

There're some questions and I see no Cypress tests right now as @mattwr18 has suggested.
Should we do them together?

webapp/components/DeleteData/DeleteData.spec.js Outdated Show resolved Hide resolved
webapp/components/DeleteData/DeleteData.spec.js Outdated Show resolved Hide resolved
webapp/components/DeleteData/DeleteData.spec.js Outdated Show resolved Hide resolved
webapp/components/DeleteData/DeleteData.spec.js Outdated Show resolved Hide resolved
webapp/components/DeleteData/DeleteData.vue Outdated Show resolved Hide resolved
webapp/graphql/User.js Outdated Show resolved Hide resolved
webapp/graphql/User.js Outdated Show resolved Hide resolved
webapp/graphql/User.js Show resolved Hide resolved
@ogerly ogerly requested a review from Tirokk July 30, 2020 11:43
@Tirokk
Copy link
Member

Tirokk commented Aug 4, 2020 •
edited

I have fixed the linting …

@Tirokk
Refactore and cleanup backend tests
17a296b 
- Removed double declarations.
- Put assignments in 'beforeEach'.
ogerly
ogerly commented Aug 5, 2020
View reviewed changes
Copy link
Contributor Author

@ogerly ogerly left a comment

Choose a reason for hiding this comment

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

ok

webapp/components/DeleteData/DeleteData.spec.js Outdated Show resolved Hide resolved
webapp/components/DeleteData/DeleteData.vue Outdated Show resolved Hide resolved
webapp/graphql/User.js Outdated Show resolved Hide resolved
webapp/graphql/User.js Outdated Show resolved Hide resolved
webapp/graphql/User.js Show resolved Hide resolved
webapp/components/DeleteData/DeleteData.spec.js Outdated Show resolved Hide resolved
Tirokk
Tirokk approved these changes Aug 5, 2020
View reviewed changes
Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

Hey @ogerly ,
great that you cared for this bug! 🚀🚀💫

I approve and merge. 🍀

@Tirokk Tirokk changed the title fix: #3498 Checkboxes missing on delete user account page fix: 🍰 Checkboxes Not Missing Anymore On Delete User Account Page Aug 5, 2020
@Tirokk Tirokk merged commit 7106fa8 into master Aug 5, 2020
Human-Connection automation moved this from In progress to Done Aug 5, 2020
@Tirokk Tirokk deleted the 3498-Checkboxes_missing_on_delete_user_account_page branch August 5, 2020 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tirokk Tirokk Tirokk approved these changes

@roschaefer roschaefer Awaiting requested review from roschaefer

@rbeer rbeer Awaiting requested review from rbeer

@Mogge Mogge Awaiting requested review from Mogge

@mattwr18 mattwr18 Awaiting requested review from mattwr18

Assignees

@ogerly ogerly

Labels
bug
Projects
Human-Connection
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🐛 [Bug] Checkboxes missing on delete user account page
5 participants
@ogerly @rbeer @Tirokk @Mogge @mattwr18