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

Swik 1440 improve slide links in notifications #1055

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dpaun
Copy link
Member

@dpaun dpaun commented Oct 23, 2018

Currently, slide links in notifications point to slide-view, which is not good. Using the recently added activity parameter, content_root_id, slide links (in newly created notifications, which contain content_root_id) will point to proper path within a deck (deck-view + slide).

@kprist
Copy link
Member

kprist commented Oct 23, 2018

Haven't tested it yet, but I can see you also include the spath, calculating it ? Very interesting and maybe we can use it for other things, however there are issues with this parameter in our URLs, in combination with including the revision for decks / slides. These can lead to 404 pages, for an ever-changing deck. If the deck revision is not the latest, then the URL will never break, but if we are not, then those things (the spath and the slide revision) can change.

So what I am asking, can we simply not include the spath and the slide revision? You can keep the deck revision for now. Or are there other considerations for that?

To be clear, we can store things in services however we want, I'm just talking about the URLs.

@dpaun
Copy link
Member Author

dpaun commented Oct 23, 2018

Can I use urls like these: /deck/ {deck id }/slide/ {slide id} ? Without the path part in it? It seems to work. But it doesn't work if there is no slide revision number.

@kprist
Copy link
Member

kprist commented Oct 23, 2018

You should be able to do that, in theory. Can you give me an example where removing the slide revision fails ?

@kprist
Copy link
Member

kprist commented Oct 23, 2018

Hmm this looks like a bug / regression, let me check it

@kprist
Copy link
Member

kprist commented Oct 24, 2018

Not exactly a bug or regression, platform has added features where spath and revisions are needed again, so path generation is ok for now. It's a bit overkill we need to do a whole decktree request however, so I opened ticket https://slidewiki.atlassian.net/browse/SWIK-2516 and branches in deck and platform wherein the idea is similar to what you do here, i.e. I recreate the spath and revisions, but for the whole page, not just for the sake of the notifications links.

If you think it's important to merge, let me know. Since it's not UI-related, it could wait.

On another note, please also apply the same URL strategy not just for slides, but also for subdecks, where applicable.

@dpaun
Copy link
Member Author

dpaun commented Oct 24, 2018

It can wait, I agree.
I will apply it for subdecks in the next commit.

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

Successfully merging this pull request may close these issues.

2 participants