Andras/BoundlessFlowCampus2K
3
6
Fork 0
Code Issues 142 Pull requests 15 Projects Releases Packages Wiki Activity Actions 2

#339-Big-card-for-all-temperature-sensors #385

Closed
a24danwe wants to merge 0 commits from #339-Big-card-for-all-temperature-sensors into team_2_week_2
pull from: #339-Big-card-for-all-temperature-sensors
merge into: Andras:team_2_week_2
Conversation 7 Commits - Files changed -
a24danwe commented 2026-04-17 07:23:11 +00:00
Collaborator
Copy link

Issue #339

Added function to retrieve all sensors in tempService
Added TemperatureAllView.js
Added useTemperatureAll.js
Added TemperatureAllView to the dropdown in Dashboard.js

Issue #339 Added function to retrieve all sensors in tempService Added TemperatureAllView.js Added useTemperatureAll.js Added TemperatureAllView to the dropdown in Dashboard.js
a24danwe commented 2026-04-17 07:24:03 +00:00
Author
Collaborator
Copy link

Waiting for c24elipe to review

Waiting for c24elipe to review
c24elipe commented 2026-04-17 10:43:14 +00:00
Collaborator
Copy link

Small problems regarding formatting in service/tempService/Program.cs.

Small problems regarding formatting in service/tempService/Program.cs.
c24elipe commented 2026-04-17 10:43:32 +00:00
Collaborator
Copy link

Formal review comes later, can't do one now

Formal review comes later, can't do one now
c24elipe commented 2026-04-17 14:41:19 +00:00
Collaborator
Copy link

Review on #385

Tested on CahyOS & Linux Mint using Firefox

  • Running the application:

    • ERRORS: An error that was found was in relation to a 505 error when trying to fetch the data from the back-end. However, this issue was found to be the result of running this application with the Brave browser or some ad-blocking plugins on Firefox. Once fixed, all such errors disappeared.

    • Tested: This application was tested by first running it normally and seeing that all Docker containers start as they should. After they were found to indeed work correctly, the Firefox browser was opened to the frontend container, and from the dropdown, the relevant "TemperatureAllView" was added correctly. I also then tried to format the relevant files using Prettier and noted the file changes using git. (red being the current version)

image

Inspected the following files

OBS: No new dependencies from outside of this repo were found or added.

  • frontend/src/features/temperature/TemperatureAllView.js

    • The indentation and style standards are almost completely followed by the Prettier-enforced formatting. The only formatting not allowed was around the import of "useTemperatureAll" at the top of the file.

    • The Naming convention was followed.

    • No comments are present. But I, as the review belive that the code is self-documenting enough to get away with it.

    • The code quality is acceptable. Not too hard to understand, and it does what it needs to.

    • A general issue I found with all Temperature components was that they include CSS in their JS files, but since all others do it, and it seems to be the standard, I deem it okay.

  • frontend/src/components/Dashboard/Dashboard.js:

    • All that was done in this file was the import and addition of the new TemperatureAllView to the dropdown list for the frontend.

    Since this is such a small change, with no formatting errors, that seems to be copied and pasted from other code, is that all I will write on the matter?

  • frontend/src/features/temperature/useTemperatureAll.js:

    • In this file, the standard sharedFeatures.js code is used as intended without any indentation or usage errors.

  • service/tempService/Program.cs:

    • A new endpoint was added here from lines 78->107.

    • The indentation and style standard is again almost correct. The first error shown is that the whole function mapping is a little weirdly formatted. The main offender is "app.MapGet", that is for more space to the right than it should be to follow the standards set.
      image
      Besides, were there errors, no other style standard blunders were found.

    • To follow the Naming convention, the variable "sql" (found on line 81) is named incorrectly all over the file. The correct name should be "Sql" or "SQL" ( depending on preference ) due to the code standard stating "- Use PascalCase for constant names, both fields and local constants."

    • The code quality is good, adequate, and is on par with the rest of the file.

    • Comments are placed correctly and only where they are needed, on more complex logic.

What needs to be done 

What needs to be done is a simple formatting on all relevant files, and if the "sql" naming convention is deemed to be correct, then a small refactor of the variable in service/tempService/Program.cs needs to be refactored.

Conclusion 

Besides small flaws, this code looks good, and if fixed, can be merged.

# Review on #385 ## Tested on CahyOS & Linux Mint using Firefox - Running the application: + **ERRORS**: An error that was found was in relation to a 505 error when trying to fetch the data from the back-end. However, this issue was found to be the result of running this application with the Brave browser or some ad-blocking plugins on Firefox. Once fixed, all such errors disappeared. + **Tested**: This application was tested by first running it normally and seeing that all Docker containers start as they should. After they were found to indeed work correctly, the Firefox browser was opened to the frontend container, and from the dropdown, the relevant "TemperatureAllView" was added correctly. I also then tried to format the relevant files using Prettier and noted the file changes using git. (red being the current version) ![image](/attachments/ae1d0ec3-edc8-4e4d-a50e-8e8433602c0f) ## Inspected the following files OBS: No new dependencies from outside of this repo were found or added. - frontend/src/features/temperature/**TemperatureAllView**.js - The **indentation and style standards** are almost completely followed by the Prettier-enforced formatting. The only formatting not allowed was around the import of "useTemperatureAll" at the top of the file. - The **Naming convention** was followed. - No **comments** are present. But I, as the review belive that the code is self-documenting enough to get away with it. - The **code quality** is acceptable. Not too hard to understand, and it does what it needs to. - A **general issue** I found with all Temperature components was that they include CSS in their JS files, but since all others do it, and it seems to be the standard, I deem it okay. - frontend/src/components/Dashboard/**Dashboard**.js: - All that was done in this file was the import and addition of the new TemperatureAllView to the dropdown list for the frontend. Since this is such a small change, with no formatting errors, that seems to be copied and pasted from other code, is that all I will write on the matter? - frontend/src/features/temperature/**useTemperatureAll**.js: - In this file, the standard sharedFeatures.js code is used as intended without any indentation or usage errors. - service/tempService/Program.cs: - A new endpoint was added here from lines 78->107. - The **indentation and style standard** is again almost correct. The first error shown is that the whole function mapping is a little weirdly formatted. The main offender is "app.MapGet", that is for more space to the right than it should be to follow the standards set. ![image](/attachments/90f34980-62ba-4b77-8bf8-73bb972395c1) Besides, were there errors, no other style standard blunders were found. - To follow the **Naming convention**, the variable "sql" (found on line 81) is named incorrectly all over the file. The correct name should be "Sql" or "SQL" ( depending on preference ) due to the code standard stating "- Use PascalCase for constant names, both fields and local constants." - The **code quality** is good, adequate, and is on par with the rest of the file. - **Comments** are placed correctly and only where they are needed, on more complex logic. ## What needs to be done  What needs to be done is a simple formatting on all relevant files, and if the "sql" naming convention is deemed to be correct, then a small refactor of the variable in service/tempService/Program.cs needs to be refactored. ## Conclusion  Besides small flaws, this code looks good, and if fixed, can be merged.
image.png
18 KiB
image.png
9.1 KiB
a24danwe commented 2026-04-17 15:19:18 +00:00
Author
Collaborator
Copy link

Thank you for the review!

I think I have fixed the formatting issues and also the naming convention of sql -> Sql.

Also the reason for some code being copied in "Since this is such a small change, with no formatting errors, that seems to be copied and pasted from other code, is that all I will write on the matter?" was because I had to redo the branch and so I copied all my code and pasted it in, that was maybe also the reason why the indentation was wrong in some areas.

Thank you for the review! I think I have fixed the formatting issues and also the naming convention of sql -> Sql. Also the reason for some code being copied in "Since this is such a small change, with no formatting errors, that seems to be copied and pasted from other code, is that all I will write on the matter?" was because I had to redo the branch and so I copied all my code and pasted it in, that was maybe also the reason why the indentation was wrong in some areas.
c24elipe approved these changes 2026-04-20 08:05:12 +00:00
c24elipe left a comment
Collaborator
Copy link

I have rechecked, and all flaws found are now fixed.

I have rechecked, and all flaws found are now fixed.
a24danwe commented 2026-04-20 09:08:28 +00:00
Author
Collaborator
Copy link

Conflict has been resolved and been merged into team_2_week_2. PR can be closed.

Conflict has been resolved and been merged into team_2_week_2. PR can be closed.
c24elipe closed this pull request 2026-04-20 09:16:57 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
c24elipe
Labels
Clear labels   
_CRITICAL_

Only use this label for critical tasks that are project-scope,and must be addressed ASAP.

  
API

   
app

   
backEnd

   
Blocked-waiting-for-further-changes

   
bug

Something is not working

  
bug-only-on-server-for-mobile-not-webpage

   
Bug-Report-After-Merge

   
cleanup

Issues to cleanup the codebase (restructure folders, remove deprecated or smelly code)

  
close

Issue to close

  
design

   
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
feature request

   
frontEnd

   
further-changes-needed

   
future-problem-not-fixint-this-period

   
help wanted

Need some help

  
invalid

Something is wrong

  
last-week-issue-to-fix

   
library

   
low-priority

   
needs input

Issue needs input from product owner or customer

  
needs review

issue is in need of review

  
not-implemented.

   
project documentation

   
question

More information is needed

  
research

   
reviewed

This Issue has been reviewed

  
Script

   
security

use this for security-related issues

  
SQL

  
style

   
Team 1

Team 1 is responsible for this issue

  
Team 2

Team 2 is responsible for this issue

  
team leaders

   
test-creation

   
testing

   
topLevel

Reserved for top-level, project-wide issues.

  
unassigned

   
Under-review

   
wontfix

This won't be fixed

No labels
_CRITICAL_
API
app
backEnd
Blocked-waiting-for-further-changes
bug
bug-only-on-server-for-mobile-not-webpage
Bug-Report-After-Merge
cleanup
close
design
duplicate
enhancement
feature request
frontEnd
further-changes-needed
future-problem-not-fixint-this-period
help wanted
invalid
last-week-issue-to-fix
library
low-priority
needs input
needs review
not-implemented.
project documentation
question
research
reviewed
Script
security
SQL
style
Team 1
Team 2
team leaders
test-creation
testing
topLevel
unassigned
Under-review
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
a24danwe
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Andras/BoundlessFlowCampus2K!385
No description provided.