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

Feature/orebro tsm 3 #1402

Merged
merged 69 commits into from
Nov 20, 2023
Merged

Feature/orebro tsm 3 #1402

merged 69 commits into from
Nov 20, 2023

Conversation

Sunix71
Copy link
Contributor

@Sunix71 Sunix71 commented Sep 9, 2023

We have made an attempt to develop a solution for issue 881 (#881). We have added folder functionality to Dokumenthanterare 2.0 so that users can create folders and place their documents in these folders or in the root folder if desired (…\App_Data\documents).

This has meant that we have had to change the API's endpoints so that it also searches for a directory and not just a document.
We have done this in a way so it should not affect existing configurations that are already set up.

Since we have 2 backends, we have made it such that the administrator needs to set a configuration-key in config.json (new-admin\public\config.json) for this functionality to work.
The key looks as follows: "use_document_folders": true
And should be located under Documenthandler and mappsettings in the config file. The system should work without this key, but then without the functionality.

The map configuration file has received an additional attribute named folder, it looks like this:
{
"title": "Exempeldokument2",
"folder": "",
"document": "exempeldokument2",
"color": "",
"icon": {
"materialUiIconName": "",
"fontSize": "large"
},
"maplink": "",
"link": "",
"menu": []
}
However, the system should function without this attribute.

Copy link
Contributor

@jesade-vbg jesade-vbg left a comment

Choose a reason for hiding this comment

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

I've had a quick look and noticed some stuff.
Note that I have not yet tested.

new-admin/src/models/documenteditor.js Show resolved Hide resolved
new-admin/src/models/documenteditor.js Outdated Show resolved Hide resolved
new-admin/src/models/documenteditor.js Outdated Show resolved Hide resolved
new-admin/src/models/documenteditor.js Outdated Show resolved Hide resolved
new-admin/src/models/menuEditorModel.js Show resolved Hide resolved
new-admin/src/views/documenteditor.jsx Outdated Show resolved Hide resolved
new-admin/src/views/documenteditor.jsx Outdated Show resolved Hide resolved
new-admin/src/views/tools/MenuEditor/menuEditor.jsx Outdated Show resolved Hide resolved
@Sunix71
Copy link
Contributor Author

Sunix71 commented Sep 11, 2023

I've gone through all your comments and hope I haven't missed anything. Made a new push now. Grateful for your time!

@jesade-vbg
Copy link
Contributor

I've gone through all your comments and hope I haven't missed anything. Made a new push now. Grateful for your time!

Great. And you noticed my url examples wasn't optimized aswell, sorry :) Good.

Copy link
Member

@jacobwod jacobwod left a comment

Choose a reason for hiding this comment

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

Over all, it seems as you've only put this into the v1 API. I'm not sure if it should be added to V1 at all (as we've recently stated that V1 should maintain compatibility with the old .NET backend), but if you feel like it's good, go for it. Nevertheless, this should also be added to the v2 part of new-backend.

Don't forget to merge in the latest changes in develop before you begin.

new-backend/server/apis/v1/services/informative.service.js Outdated Show resolved Hide resolved
@Sunix71
Copy link
Contributor Author

Sunix71 commented Oct 4, 2023

Over all, it seems as you've only put this into the v1 API. I'm not sure if it should be added to V1 at all (as we've recently stated that V1 should maintain compatibility with the old .NET backend), but if you feel like it's good, go for it. Nevertheless, this should also be added to the v2 part of new-backend.

Don't forget to merge in the latest changes in develop before you begin.

I have added the features to V2 now and pushed (merged develop before I started). I have not yet removed the functionality from V1; that will be the next step, I think. Then, I need to look into path.join to see how one might construct the paths in a different way?! If you have any suggestions please respond :-)

@jacobwod
Copy link
Member

jacobwod commented Oct 4, 2023

Sure!

Search and you will find.

Basically: don't assume that the delimiter is a /, use path.join() where you'd use a /.

@Sunix71
Copy link
Contributor Author

Sunix71 commented Oct 4, 2023

I've restored V1 and removed / and used path.join instead. There are perhaps more issues to resolve. Perhaps optimizing code in informative?
Thank you for your time!

Copy link
Member

@jacobwod jacobwod left a comment

Choose a reason for hiding this comment

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

I left some comments in code, but it's, mostly the same problem and you have it in more places than those I've marked.

We've tested the branch though and it works fine, so as soon as the code issues are fixed, this should be ready for merge.

jacobwod and others added 3 commits November 1, 2023 10:58
…ferent endpoints. Ensured that it works with current Admin, even though some of them are technically incorrect (and marked with a comment, as such).
@Sunix71
Copy link
Contributor Author

Sunix71 commented Nov 2, 2023

I have gone through the code and made some changes, please have a look through it again, thank you!

A question regarding config.json in new-admin public: What should it look like? Right now, my parts are with :3002/api/v1 while others are with :55630, which might be a bit confusing, right?
How would you like me to proceed with this?

And one more question :-)
I did this in new-backend/informative:

if (typeof folder === "undefined") {
throw new Error("Folder is undefined");
}
But I'm a bit uncertain if that's what we want?!
Maybe we want to set folder to an empty string if it's undefined?

@Hallbergs Hallbergs self-requested a review November 20, 2023 14:20
@jacobwod
Copy link
Member

While reviewing and testing, we noticed that it seems impossible to set starting document if that document is in a folder. Perhaps there's another way to do than what we've tried, if so please document it (in Admin). Thanks @Sunix71.

image

@jacobwod jacobwod merged commit 8aef7fc into develop Nov 20, 2023
@jacobwod jacobwod deleted the feature/orebro_tsm_3 branch November 20, 2023 14:25
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