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

added school edit pages, utils, tests, stories #36

Merged
merged 4 commits into from
May 31, 2024
Merged

added school edit pages, utils, tests, stories #36

merged 4 commits into from
May 31, 2024

Conversation

30912hyl
Copy link
Contributor

@30912hyl 30912hyl commented May 24, 2024

Worked on Issue #33, which I split from EPIC issue #4, which sets up the frontend for School CRUD

https://organic-30912hyl.dokku-11.cs.ucsb.edu/

Since school Create has not been implemented yet, in order to test this please first manually create a school entry with https://organic-30912hyl.dokku-11.cs.ucsb.edu/swagger-ui/index.html#/

Closes #33

@30912hyl
Copy link
Contributor Author

nevermind, tested the edit button and it seems to not be linked. please don't review for now!

@30912hyl 30912hyl marked this pull request as ready for review May 24, 2024 01:32
@30912hyl
Copy link
Contributor Author

ok, seems to be fixed, can review now!

@pconrad pconrad added the FIXME-deploy Please deploy this branch to a dokku dev instance and link to it in PR description label May 25, 2024
@30912hyl 30912hyl self-assigned this May 29, 2024
Xalex217
Xalex217 previously approved these changes May 29, 2024
Copy link
Contributor

@Xalex217 Xalex217 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

Please fix the commented out code.

frontend/src/tests/pages/School/SchoolEditPage.test.js Outdated Show resolved Hide resolved
frontend/src/tests/pages/School/SchoolEditPage.test.js Outdated Show resolved Hide resolved
frontend/src/tests/pages/School/SchoolIndexPage.test.js Outdated Show resolved Hide resolved
@pconrad pconrad added the FIXME-See CR Please review the comments in the code review and address them; then remove this label label May 29, 2024
@30912hyl 30912hyl removed FIXME-See CR Please review the comments in the code review and address them; then remove this label FIXME-deploy Please deploy this branch to a dokku dev instance and link to it in PR description labels May 29, 2024
Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

Code looks good.. but when I tried this on the dokku deployment, I was unable to create a school. Therefore I was unable to test editing a school, which seems to be the main thing in this PR.

I wonder if your dokku deployment is out of date?

Or if create is in a different PR that needs to come before this one for testing purposes?

Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

I don't see any problem with the code. But this is what happens when I try to test:

pr-36

@pconrad pconrad added the FIXME-See CR Please review the comments in the code review and address them; then remove this label label May 30, 2024
@30912hyl
Copy link
Contributor Author

Code looks good.. but when I tried this on the dokku deployment, I was unable to create a school. Therefore I was unable to test editing a school, which seems to be the main thing in this PR.

I wonder if your dokku deployment is out of date?

Or if create is in a different PR that needs to come before this one for testing purposes?

Oh, sorry yes that is the case! Since this requires a school to have been created already, for testing purposes this PR must come after EPIC Issue #4 F1.1. Another option that I tried and confirmed was to test on localhost, creating a school using Swagger and attempting to use the edit button there. It may also be possible to edit the initial database for Dokku, however for simplicity's sake, should I leave this PR as it is with just localhost screenshots, and create a new PR later for the dev deployment once EPIC Issue #4 F1.1 is done?

@30912hyl
Copy link
Contributor Author

Ok, I updated the PR description so that anyone can go to Swagger first and create a school entry. That way there is an entry present to test "edit button"

@30912hyl 30912hyl removed the FIXME-See CR Please review the comments in the code review and address them; then remove this label label May 31, 2024
Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

Delete appears to be broken:

Uploading delete-is-broken.gif…

Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

Delete is broken:

delete-is-broken

@pconrad
Copy link
Contributor

pconrad commented May 31, 2024

Make sure that the backend api in your branch matches this:

  const deleteMutation = useBackendMutation(
         cellToAxiosParamsDelete,
         { onSuccess: onDeleteSuccess },
         ["/api/schools/all"]
);

And make sure that you've tested delete.

Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

I am reluctantly approving this even though it has a broken Delete button. We'll just have to hope that the next PR fixes that.

@pconrad pconrad merged commit 3e91808 into main May 31, 2024
9 checks passed
@pconrad pconrad added the 5 label May 31, 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.

F1.2: Building Edit button
3 participants