Fix categories - [merged]

[cswendrowski]

In GitLab by @adrien.schiehle on Nov 3, 2021, 11:09

Merges fix_categories -> categories

First commit

For fixing importAll and some hierarchy problems.

We had in fact two Root folder when importing. ( The parent tree node was also a folder named Root )

I decided to let one here.
I tested : If they want to remove the upper folder after moving the other one outside of it, it still works. Link remains

Second commit

For visibility buttons. I think that when you tried to simplify things, you found that I didn't use entry.visible and chose to use it.

But its purpose slightly differs from what you thought : WA Panel control is only for GMs, and those buttons gives him info on what is visible by other players? It's great to keep track of what you have shared to your players until now.

It can also be used when you have a NPC who have evolved and its description changed.

• Hide its description on prior category
• Set it visible on the new one.

(For campaign data, I use a WA category for each act)

[cswendrowski]

In GitLab by @adrien.schiehle on Nov 3, 2021, 11:12

added 1 commit

• 58b3ee9 - Missing labels

Compare with previous version

[cswendrowski]

I have merged this code change because it (unfortunately) is too large and touches on too many issues at once for me to effectively review it step-by-step. In the future we need to work on smaller MRs that fixes one issue at a time.

I will leave some comments for things that I think are important to improve upon after this code is merged before we cut a module release.

[cswendrowski]

We should build a module-level cache of categories which persists throughout the user session and is refreshed on-demand rather than specifically passing in categories in this way.

[cswendrowski]

Factoring this method out does not seem to have been a value-adding change since we only use it here and this method already specifically deals with article import.

[cswendrowski]

This isn't a clear/informative name for this const.

[cswendrowski]

We aren't really "refreshing" here so much as re-associating.

[cswendrowski]

We created a separate _sortArticles helper here which is not used anymore. Why?

[cswendrowski]

Why do/should we care that a category is empty?

[cswendrowski]

What does it mean in this context for an article not to be "visible", we receive the data from the WA API so why would it not be "visible"?

[cswendrowski]

Is this change done in order to import all articles under a parent category recursively? If so... that is a useful feature to add but new features like this should not be part of a MR like this. It becomes way too much to review at once. We should add these feature changes one at a time.