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

KAS-4588: added util to add hash in documentviewermodal #2076

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

StuyckRuben
Copy link
Contributor

Copy link
Contributor

@ValenberghsSven ValenberghsSven left a comment

Choose a reason for hiding this comment

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

Need to test with some real world examples of legacy docs / notulen docs / decision docs / minutes etc if no errors occur and what the edges cases are.

Comment on lines 2 to 3
const hash = pieceName.replaceAll(" ", "-");
window.location.hash = hash;
Copy link
Contributor

Choose a reason for hiding this comment

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

replacing spaces with dashes could lead to excessive dashes
VR-PV-2024/7---ALLE-BESLISSINGEN no idea if that is ok.

Any other symbols we need to replace (escape characters perhaps? & " ') If any are present. They might just get translated to symbols %20
need to check existing data for special cases.

Copy link
Member

Choose a reason for hiding this comment

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

We should be okay just stripping out the following characters:

     = | ; | / | # | ? | : | space 

https://www.w3.org/Addressing/URL/5_URI_BNF.html

Others are either escaped (as you say, with stuff like %20) or they're just allowed

Copy link
Contributor

@ValenberghsSven ValenberghsSven Apr 25, 2024

Choose a reason for hiding this comment

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

as far as I can tell, only space needs to be replaced. that gets transformed into a symbol when not replaced.
(in my screenshot, I replaced the spaces with underscore which look cleaner i.m.o.)
so - instead of ---

image

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I noticed here is that the downloaded file uses file name, not piece name.
Alvin was next to me when I was testing this. We should be using the piece name instead of the file name I think
Will discuss with Tom and probably make a ticket.
I think we do use piece name when downloading all documents from agenda, but in some places, we are offering the download with a different name than we see in the app (the former which cannot be changed)

Comment on lines 46 to 47
const shortPieceName = new VRDocumentName(model?.name).vrNumberWithSuffix();
setHash(shortPieceName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Problem here is that there are multiple types of documents, some have their own VR numbering parsing
image

Copy link
Contributor

Choose a reason for hiding this comment

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

so in many cases we would fall back to just the full name, which might not be desired?
example VR PV 2024/7 - ALLE BESLISSINGEN which actually makes sense, because this is a different file than VR PV 2024/7

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK this feature/ticket is meant to enable the document stamping to be "visible" in the UI withouth being stamped, so that we only need to stamp when downloading/printing.

I.e. this doesn't really matter for beslissingsfiches, notulen, etc. so we might even want to disable this for those document types.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, we'll leave this as-is. We try to parse it using the VR doc name parser. If that doesn't work, we show the full name.
Should be ok. We always show something normally.

sergiofenoll and others added 3 commits April 11, 2024 20:53
Copy link
Contributor

@ValenberghsSven ValenberghsSven left a comment

Choose a reason for hiding this comment

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

@sergiofenoll that location replace fix you did was not working, it caused the app to just not transition into the documents route.
ex. clicking on an agendaitem doc tried to transition, but ended up back on the agendaitem documents page with the hash applied to that address.
In order to fix that I had to move the hash setting to the loading action.

Also, this hash was gone as soon as we switched tabs since the model is not refreshed in that case (queryParams are set up that way)

So we need some call to the router for that.
I got some fixes locally, will push these and you can give your feedback on it.
With these fixes:

  • hash is applied the first time on the @action loading of the route
  • hash is reapplied on switching tab (with a timeout since I couldn't figure out how to catch the "replace: true" portion of the queryParams. I tried a 'routeWillChange' but that was too aggressive.)
  • had to set model in router as a local param because of potential name updates. After editing we don't update the hash, but switching tabs works to update the hash. Unsure if we want to do some actions up to retrigger the hash after editing the name. Seems overkill tbh)
  • I changed the util a bit since I hit the util when pieceName was not loaded yet. The util should be able to handle a null value and just do nothing.
  • Changed the replace from spaces to underscore. It looks cleaner but we can discuss this.

@ValenberghsSven
Copy link
Contributor

ValenberghsSven commented Jun 12, 2024

Some work went in this, but it might no longer be needed now that we are actually stamping documents?
@tdn
If we do want it, need to update and check again. Some tests were failing here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants