ida-arbeitszeit / arbeitszeitapp

A webapp for labour-time calculation.
https://arbeitszeitapp.readthedocs.io
GNU Affero General Public License v3.0
33 stars 4 forks source link

Create ChangeUserEmailAddressPresenter #1031

Closed sloschert closed 3 weeks ago

sloschert commented 1 month ago

This presenter is intended to be used in the change_email_address POST route in arbeitszeit_flask/user/routes.py and presents the response of the ChangeUserEmailAddressUseCase.

Contributes to #728.

Plan-ID: 371825c0-3112-47cd-9cb0-fea799fad3ee (2x)

4lm commented 3 weeks ago

👀 I'll have a look at this PR.

sloschert commented 3 weeks ago

Hey, thanks for your review and remarks! 1) Before I'll have a look at your remarks: Merging is done by the requesting person after approval. 2) I think it's just a mistake that you don't have write access. We can talk about this later today.

sloschert commented 3 weeks ago

@4lm

sloschert commented 3 weeks ago

@seppeljordan I registered 2x consumption

4lm commented 3 weeks ago

@4lm: ... the ViewModel should only consist of string and bools (if I remember Uncle Bob's intention with the ViewModel approach correctly). Strictly viewed, the None type should then be an empty string ... ?

I'm totally OK with the merge but want to clarify my point further because I believe I didn't get the point across here, and yesterday in our meeting (I was really tired, and my verbal communication skills seemed to be on vacation without giving me a proper heads up).

My "minor" issue with using "None" is only specific to the ViewModel in a Clean Architectureâ„¢ context. The ViewModel properties returned by the presenter should only consist of strings, bools, and structural data types (think List, Set, Dict, or @dataclass in a Python context) holding those strings and bools. They also should not be union types, as @Amittai-Aviram correctly noted. The ViewModel's intended use is in the outer framework (Flask) layer for rendering the UI, in our case, HTML via Flask, even if Flask sanitizes "None" correctly to an empty string ("). The data contained in the VideoModel (as I understood Uncle Bob and @seppeljordan in the past) should be as agnostic and basic as possible and shouldn't leak programming language-specific types as data.

I looked through the source code, and I think the implementation in list_all_cooperations_presenter.py is a good representation of what I mean.

sloschert commented 3 weeks ago

@4lm: ... the ViewModel should only consist of string and bools (if I remember Uncle Bob's intention with the ViewModel approach correctly). Strictly viewed, the None type should then be an empty string ... ?

I'm totally OK with the merge but want to clarify my point further because I believe I didn't get the point across here, and yesterday in our meeting (I was really tired, and my verbal communication skills seemed to be on vacation without giving me a proper heads up).

My "minor" issue with using "None" is only specific to the ViewModel in a Clean Architectureâ„¢ context. The ViewModel properties returned by the presenter should only consist of strings, bools, and structural data types (think List, Set, Dict, or @dataclass in a Python context) holding those strings and bools. They also should not be union types, as @Amittai-Aviram correctly noted. The ViewModel's intended use is in the outer framework (Flask) layer for rendering the UI, in our case, HTML via Flask, even if Flask sanitizes "None" correctly to an empty string ("). The data contained in the VideoModel (as I understood Uncle Bob and @seppeljordan in the past) should be as agnostic and basic as possible and shouldn't leak programming language-specific types as data.

I looked through the source code, and I think the implementation in list_all_cooperations_presenter.py is a good representation of what I mean.

Thanks for raising this again, I agree. Soon I will push the Pull Request with the broader context (the usage of the presenter in the flask layer/view function), then we will have an opportunity to discuss this further or rectify it.

4lm commented 3 weeks ago
  • Regarding testing the url creation in flask: Have you seen the module tests/flask_integration/test_url_index.py? Does this answer your question? Moreover: url_for returns str in my nix setup.

Yes, this does answer it — thank you for clarifying! Also, please note that my remark concerning well-formed URL string was a little brainfart because the framework (Flask) layer is not the interface and adapter layer in our project, aka the "web" layer of the presenter. Therefore, something like "url_index" must be a string (as the data type and not a specific URL encoded string type to be agnostic), and the outer Flask layer is responsible for creating well-formed URLs from those strings; what url_for does, and what is tested by the tests you are mentioning.

Something seems incorrect with my nix develop setup in combination with VS Code. url_for returns Any because Flask is not imported in VS Code. Therefore, IntelliSense is not properly working. So my remark on "url_for returns Any" was a false positive.