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

Draft & dummy implementation of account removal #274

Merged
merged 5 commits into from
Sep 2, 2016

Conversation

fedys
Copy link
Collaborator

@fedys fedys commented Aug 22, 2016

refs gh-103

@fedys
Copy link
Collaborator Author

fedys commented Aug 22, 2016

Please let me know your thoughts on current dummy implementation.

@coveralls
Copy link

coveralls commented Aug 22, 2016

Coverage Status

Changes Unknown when pulling a9cf89b on fedys:issue-103 into * on cross-solution:develop*.

@TiSiE TiSiE added this to the v0.27 milestone Aug 22, 2016
@TiSiE TiSiE self-assigned this Aug 22, 2016
@TiSiE
Copy link
Member

TiSiE commented Aug 22, 2016

I like the concept to push the responsibilities of listing and removing dependent items to the modules.
But maybe an event driven approach would be more flexible and less complex...
Modules could separate the logic of listing and removing items in different listener if they want, and the Auth module does not need to provide an plugin manager, but uses a dedicated event manager, which is created relatively easy, as all necessaries are already implemented: (see #234 (comment) /the last three paragraphs)

And although it is tempting for its simplicity to use a static method as factory, we should use dedicated factory classes, because of SoC principle.

@coveralls
Copy link

coveralls commented Aug 23, 2016

Coverage Status

Changes Unknown when pulling a6c8a3c on fedys:issue-103 into * on cross-solution:develop*.

@fedys
Copy link
Collaborator Author

fedys commented Aug 23, 2016

@TiSiE
I made all refactorings you suggested and you were right. It is more simple and flexible. I will avoid using static method factories in the future (although there were also performance reason to use static methods). Let me know if you are happy with the current draft and I will continue with real implementation.

@TiSiE
Copy link
Member

TiSiE commented Aug 23, 2016

Looks good, go ahead! 😃

@fedys
Copy link
Collaborator Author

fedys commented Aug 24, 2016

Let me know if you are happy with the current implementation of related objects listing. If yes, I will supply unit tests and then we can move to the second step, related objects removal.

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Changes Unknown when pulling b29839f on fedys:issue-103 into * on cross-solution:develop*.

@TiSiE
Copy link
Member

TiSiE commented Aug 24, 2016

Code is alright. @cbleek must review the UI

@coveralls
Copy link

coveralls commented Aug 29, 2016

Coverage Status

Changes Unknown when pulling 73add93 on fedys:issue-103 into * on cross-solution:develop*.

@cbleek
Copy link
Member

cbleek commented Aug 31, 2016

Am 24.08.2016 um 17:09 schrieb Mathias Gelhausen:

Code is alright. @cbleek https://github.com/cbleek must review the UI

UI is OK. @fedis please delete the found relations/entities.

Regards,

Carsten

@coveralls
Copy link

coveralls commented Sep 1, 2016

Coverage Status

Changes Unknown when pulling 923774a on fedys:issue-103 into * on cross-solution:develop*.

@c-mutz c-mutz merged commit 923774a into cross-solution:develop Sep 2, 2016
c-mutz pushed a commit that referenced this pull request Sep 2, 2016
* Add error handling during removal operation.
* Inactivate user entity (via status)
* Redirect to start page on success.

(refs #103, #274)
c-mutz pushed a commit that referenced this pull request Sep 2, 2016
* pr-274:
  Implement user account removal
  Implemented removing of related objects for account removal refs gh-103
  Unit tests for account removal object listing refs gh-103
  Real implementation of account removal object listing refs gh-103
  Refactorized dummy implementation of account removal refs gh-103
  Draft & dummy implementation of account removal refs gh-103

Close #103, #274
@TiSiE TiSiE removed the in review label Sep 2, 2016
@fedys fedys deleted the issue-103 branch September 5, 2016 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants