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

Fixed/ Logged out error modal. #3054

Closed
wants to merge 6 commits into from

Conversation

letscodedanish
Copy link
Contributor

Fixes #3053

Changes:

With this update, the downloadSketch function now ensures that the project is only exported when the conditions regarding the logged-in user and ownership of the project are met. This helps prevent unnecessary API requests and improves the user experience by handling the download functionality more efficiently.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #[issue-number]

Comment on lines 43 to 46
dispatch(autosaveProject());
exportProjectAsZip(project.id);
if (authenticated && project.owner === authenticated.userId) {
exportProjectAsZip(project.id);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you've got the if checking a little bit backwards here. We want to allow downloading of other people's projects. But we do not want to call save functions on other people's projects.

Should be more like

 if (authenticated && project.owner === authenticated.userId) {
  dispatch(autosaveProject());
 }   
 exportProjectAsZip(project.id);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lindapaiste ,I've revised the downloadSketch function to ensure that we only trigger the autosave functionality if the user is authenticated and they are the owner of the sketch. Here's the updated implementation:

function downloadSketch() {
if (authenticated && project.owner === authenticated.userId) {
dispatch(autosaveProject());
}
exportProjectAsZip(project.id);
}

Copy link
Contributor Author

@letscodedanish letscodedanish Feb 27, 2024

Choose a reason for hiding this comment

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

@lindapaiste , And I think this could also be done , correct me if I'm wrong

if (authenticated && project.owner === authenticated.userId) {
dispatch(autosaveProject());
exportProjectAsZip(project.id);
} else {
exportProjectAsZip(project.id);
}

This code block first checks if the user is authenticated and if the current user is the owner of the project. If both conditions are met, it triggers the autosaveProject function before exporting the project as a zip file. If the conditions are not met, it directly exports the project as a zip file without triggering the autosave function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should run your code locally and make sure that it works before submitting a PR. authenticated.userId will not work because authenticated is a boolean value -- it is not the user object. project.owner is not the id -- it's the entire user object for the owner. We need a different conditional check here. Probably authenticated && canEditProjectName would do it? But we need to try it out and check the various situations, like if this is a new sketch that has not been saved before.

@raclim raclim added the Bug label Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logged out error modal pops up when downloading another user's sketch
3 participants