Conversation
|
@categulario I see that there are unfinished things here, because it's WIP, let's finish all features before creating the PR to develop. You can also cherry pick some finished changes and create a PR with that if you want, like changes in the router dependency or in the database/models, as long as the existing functionality keeps working. |
emi420
left a comment
emi420
left a comment
There was a problem hiding this comment.
Great work @categulario !! There are just a few things to be fixed before approving the PR.
- Live compatibility with new code. This feature is now broken because the map can't be created after changes on endpoints.
- Bug with tags
- Track media files ownership
- Small naming issue and missing translation
| } else if (message.file.endsWith("mp4")) { | ||
| content = <video controls className="popupVideo" alt={message.file} src={message.file} /> | ||
| } else if (message.file.endsWith("jpg")) { | ||
| } else if (message.file.endsWith("opus")) { |
There was a problem hiding this comment.
Good catch! this is for the Live version and needs to be solved ASAP :)
| </div> | ||
|
|
||
| { loading ? <> | ||
| Uploading media... |
There was a problem hiding this comment.
This text needs translation, could you add FormattedMessage please?
|
|
||
|
|
||
| @api_router.post("/map") | ||
| async def create_chatmap( |
There was a problem hiding this comment.
If we are using "map" everywhere, should we use "map" instead of "chatmap"?
create_map
delete_map
...
|
|
||
| # User's Private Map Data Endpoint | ||
| @api_router.get("/map", response_model=FeatureCollection) | ||
| @api_router.get("/map/new", response_model=FeatureCollection) |
There was a problem hiding this comment.
This endpoint is currently being used for getting the only map available for the user: the Live map, which is created when linking a device using a QR code and then updated by data.process_chat_entries.
We should take this into account, maybe by doing a Post to /map/new when linking the device and having column for identifying the map as a "Live" map linked to a device.
When testing this branch and linking a device the process is failing
INFO: 172.19.0.1:63570 - "GET /v1/map/new HTTP/1.1" 401 Unauthorized
| } for map, count in maps] | ||
|
|
||
|
|
||
| @api_router.post("/map/media") |
There was a problem hiding this comment.
We should keep tracking of which file belongs to which map, like we do in uMap, so we can then remove media files when removing a map. Would make sense to add a UploadedFiles table?
We can also take advantage of this for blocking access to files when the map is private.
This is what we have in uMap:
| return {"map_id": map_id, "sharing": map_obj.sharing.value} | ||
|
|
||
|
|
||
| @api_router.get("/media/{filename}", response_class=StreamingResponse) |
There was a problem hiding this comment.
Once we add the relation between files and maps, we should add access control to these files. Media files will be available only if the map is public of logged user is owner of the map. Another way to do this is to add ownership to each file.
| const tags = data.features.reduce((accumulator, currentValue) => { | ||
| if (currentValue.properties.tags) { | ||
| currentValue.properties.tags.forEach(tag => { | ||
| currentValue.properties.tags.split(",").forEach(tag => { |
There was a problem hiding this comment.
There's bug here, I see this error when trying to add tags:
2 participants
A few things happen here. All surrounding the idea of saving a map.
GET /mapandPOST /map) and an old one is renamed (GET /map/new). The rename allows for a more restful interface.react-router-domis replaced byreact-router.SaveButtonis renamedDownloadButtonand a newSaveButtonis created.Headercomponent is refactored to receive buttons as children, thus simplifying its logic a lot.MapListpage is created containing the table of maps that the user has created.