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

Dop develop #937

Merged
merged 57 commits into from
Dec 17, 2021
Merged

Dop develop #937

merged 57 commits into from
Dec 17, 2021

Conversation

sweco-semtto
Copy link
Contributor

A PR for document handler. Added new tags and updated the print function.

sweco-semtto and others added 30 commits September 27, 2021 10:37
page break after table of contents
@jacobwod
Copy link
Member

Correct me if I'm wrong but this looks like a bulk PR?

My understanding was that in the begging of the project we decided that you'd push smaller changes more frequently?

@sweco-semtto
Copy link
Contributor Author

You are right that this PR is a big one and you are also right that our intention was different. Unfortunately, it was not a feasible course of action that we could apply.

@Hallbergs
Copy link
Member

Just a quick update. Will take a look at this tomorrow!

@sweco-semtto
Copy link
Contributor Author

Great, please let me know if I can help.

Copy link
Member

@Hallbergs Hallbergs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to start reviewing, but upon starting the Admin UI i was prompted with several warnings, see below:

image

Please take care of these, then i'll have a look at the code.

@jacobwod
Copy link
Member

The printout itself works and is looking good!

The Admin UI, however, refuses to load since you haven't implemented the new endpoints in the NodeJS Backend.
Skärmavbild 2021-12-10 kl  10 17 25

What's your opinion on this? Was handling of video and audio tags only ment to work in the .NET backend? If so, why did you add the new endpoints to the new-admin/public/config.docker.json:

    "list_videos": "/api/v1/mapconfig/listvideo",
    "list_audio": "/api/v1/mapconfig/listaudio"

This will not work and will lead to the Docker container crashing, as it's NodeJS backend-only.

@sweco-secoma
Copy link
Contributor

We have fixed the lint warning messages in admin and added routes for video and audio in the new backend. There is no file type checking for the audio/video routes (same as the existing image route).

if (Path.GetExtension(file).ToLower() == ".mp4" || Path.GetExtension(file).ToLower() == ".mov" || Path.GetExtension(file).ToLower() == ".ogg")
{
fileName = Path.GetFileName(file);
if (fileName != "layers")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To diffrate pictures from videos etc. that are in the same folder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @jacobwod is referring to the if (fileName != "layers") part. The configuration files are not in the same folder as the upload-data, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, the configuration files are not in the Upload folder, only data files such as images and videos. I guess this line is from an previous iteration of DOP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix this issue and update the PR with that fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Hallbergs Hallbergs merged commit 62b3b92 into develop Dec 17, 2021
@Hallbergs Hallbergs deleted the dop-develop branch December 17, 2021 10:58
@Hallbergs Hallbergs restored the dop-develop branch December 17, 2021 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants