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

mrc-3700 Delete session from front end storage #184

Merged
merged 18 commits into from
Sep 8, 2023
Merged

mrc-3700 Delete session from front end storage #184

merged 18 commits into from
Sep 8, 2023

Conversation

EmmaLRussell
Copy link
Contributor

@EmmaLRussell EmmaLRussell commented Sep 6, 2023

This branch adds a column to the Sessions page allowing users to delete sessions from the list - this removes the session id from local storage, and from current store metadata. The session remains in the backend, so any links to it will continue to work.

Clicking a Delete icon shows a confirm dialog before doing the actual delete - we haven't generally used this approach for deletes, but accidentally deleting a session would be pretty annoying..

The user cannot delete the current session.

@codecov
Copy link

codecov bot commented Sep 7, 2023

@EmmaLRussell EmmaLRussell marked this pull request as ready for review September 7, 2023 09:48
Copy link
Member

@richfitz richfitz left a comment

Choose a reason for hiding this comment

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

This looks and works great. The modal is not at all annoying, either

@M-Kusumgar
Copy link
Collaborator

before i start reviewing the code, found a bug, don't think it is to do with what youve done this PR but just want to confirm, if not i can make a ticket, if you do these steps:

  1. Load app
  2. Go on sensitivity tab (RHS)
  3. Go on options tab (LHS)
  4. Go to sessions and load your current session as a new one
  5. Go to options tab, you see sensitivity options when you are on the run app

I think most likely we are either not serialising the tab or we are not updating it on load!

Copy link
Collaborator

@M-Kusumgar M-Kusumgar left a comment

Choose a reason for hiding this comment

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

This works very well! Just a small comment about refreshing the sessions page but not something to stop this from being merged!

Comment on lines 207 to 209
onMounted(() => {
store.dispatch(`sessions/${SessionsAction.GetSessions}`);
store.dispatch(`${namespace}/${SessionsAction.GetSessions}`);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this is even worth it or not but you get sessions on mount however lets say you are on the sessions page and you refresh, then you will see only the old session because the new session has not been posted yet, so you can delete all the sessions you see, of course this is fine because the new session is just being posted but i wonder if you can wait for the new session post and then show the list of session, again really dont think it is that worth it but something to keep in mind

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 think that's probably ok - if you refresh then you expect to lose your current session and its special status.

@EmmaLRussell
Copy link
Contributor Author

before i start reviewing the code, found a bug, don't think it is to do with what youve done this PR but just want to confirm, if not i can make a ticket, if you do these steps:

1. Load app

2. Go on sensitivity tab (RHS)

3. Go on options tab (LHS)

4. Go to sessions and load your current session as a new one

5. Go to options tab, you see sensitivity options when you are on the run app

I think most likely we are either not serialising the tab or we are not updating it on load!

Yep, that's a good one, definitely not anything to do with these changes. I think it's probably caused by some stale state not getting updated on new session. Would you mind creating a ticket?

@EmmaLRussell EmmaLRussell merged commit 45f4d57 into main Sep 8, 2023
3 checks passed
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.

3 participants