Conversation
…_location_table.sql
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
Thanks to @danielshid for another contribution to OED. Not unexpectedly given the size of this PR, I made a number of comments within files. Here are some thoughts that don't fit nicely within files:
- The check failed because src/client/app/components/weather/EditWeatherModalComponent.tsx does not have an MPL header.
- There needs to be a migration for the changes to the preferences table including making the correct default values.
- src/server/sql/preferences/insert_default_row.sql needs values for the new preferences columns. I propose the temperature unit be celsius since area is meter.
- There is a model and sql for weather data but no route to get the data. This is going to be needed for the graphics.
- I tried to do a careful reading of all the code. I still need to sweep the TODO items within the code to decide what to do. I also need to check the old PR comments and design doc to make sure all is good but I don't think that will show much at this point.
If anything is not clear or you have thoughts then please let me know. I'm also here to help as needed.
package.json
Outdated
| "ngraph.graph": "~20.0.0", | ||
| "ngraph.path": "~1.5.0", | ||
| "nodemailer": "~6.9.13", | ||
| "openmeteo": "~1.1.3", |
There was a problem hiding this comment.
openmeteo is now at version 1.2.3.
| const response = await request(app).get('/api/weatherLocation'); | ||
| // Check for successful response | ||
| expect(response.status).to.equal(200); | ||
| expect(response.body).to.be.an('array'); |
There was a problem hiding this comment.
This would be a much stronger test if it added a location first and verified it could retrieve that location. Please either do or add a TODO.
The other two test are also weak in not checking the returned values.
| const app = require('../../app'); | ||
| const { expect } = require('chai'); | ||
|
|
||
| mocha.describe('Weather Location Routes', () => { |
There was a problem hiding this comment.
I think all the tests should be under this one describe.
| }); | ||
| }); | ||
|
|
||
| mocha.describe('POST for adding a weather location', () => { |
There was a problem hiding this comment.
This and the next test fails because the route now (correctly) checks for admin status. Thus, it needs to login in first to get the token as in other admin route tests.
src/server/test/db/enumTests.js
Outdated
| const conn = DB.getConnection(); | ||
| let serverEnum = []; | ||
| let jsObject = []; | ||
| //SQL query returning area_unit_type ENUM |
There was a problem hiding this comment.
area_unit_type -> temperature_unit_type
| resetState(); | ||
| }; | ||
|
|
||
| const compareLocations = (loc1: { [x: string]: any; }, loc2: { [x: string]: any; }) => { |
There was a problem hiding this comment.
A few thoughts on this. First, I think it is trying to see if edits were made. This is not how many other pages do it. They check each item and some use a useEffect so the save is disabled if no change was made. I'm okay with the new way as it is more general but OED should consider switching to it across pages if it is going to be used. I also think the disable of the save is better and should be used here.
| */ | ||
| const [validLocation, setValidLocation] = useState(false); | ||
| useEffect(() => { | ||
| setValidLocation(state.identifier !== '' && state.gps !== ''); |
There was a problem hiding this comment.
This is a weak check to disable save. See below on making it better.
| placeholder='gps' | ||
| disabled={true} | ||
| /> | ||
| <FormFeedback> |
There was a problem hiding this comment.
Since it cannot be changed (disabled) then I don't think form feedback is needed. The autocomplete, etc also seem unneeded (if can remove).
| req.body.note | ||
| ); | ||
| await newLocation.insert(conn); | ||
| await addWeatherDataForLocation(newLocation, conn); |
There was a problem hiding this comment.
When you add a new weather location I get a failure notice but the location is added. However, I don't think any weather data is added. I believe the issue is that the insert happens on the line above but the id is not captured and set in the newLocation so getLatestTimeStamp does not find it and it fails. However, maybe it should have been done via this and it is something else.
| autoComplete='on' | ||
| onChange={e => handleStringChange(e)} | ||
| value={state.identifier} | ||
| placeholder='Identifier' |
There was a problem hiding this comment.
OED does not use placeholders as there is a title above each input. All of these should be removed.
5 participants
Description
Refactored and added to weather data and weather location code according to the "Getting temperature data" section of the weather data design document.
Partially Addresses #1291
Type of change
Checklist
Limitations
The data that my code gets from Openmateo does not match what Openmateo outputs directly.