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

Schools Edit Page and tests, + Minor CreatePageTest error fixed #31

Merged
merged 16 commits into from
Jun 5, 2024

Conversation

SR33G33
Copy link
Contributor

@SR33G33 SR33G33 commented May 29, 2024

Closes #32
Edit Page for Admins is visible in the Schools tab when edit button is pressed

image

allows user to edit successfully
TODO: Still allows users to edit abbrev which cause some funky stuff to happen where it overwrites another table entry if the new abbrev matches an old one

image

image

This branch is currently deployed to: https://proj-organic-sr33g33-dev.dokku-12.cs.ucsb.edu/

*Note if the deployed app looks like an old one I might just have pushed a different branch to debug, shoot me a message on slack and I can deploy the correct branch

@SR33G33 SR33G33 changed the title Sreeganesh schools crud edit Schools Edit Page and tests, + Minor CreatePageTest error fixed May 29, 2024
Copy link
Contributor

@jason-rodrigues jason-rodrigues left a comment

Choose a reason for hiding this comment

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

image
image
Term RegEx doesn't seem to be enforced on update

@SR33G33
Copy link
Contributor Author

SR33G33 commented May 29, 2024

These are kind of backend issues, Ill make a new PR to fix these after this one is merged

TODO: Make the update check that the termDescription matches new TermRegex
TODO: Make sure abbrev cant be edited in update

@SR33G33 SR33G33 requested a review from jason-rodrigues May 29, 2024 22:36
@SR33G33 SR33G33 force-pushed the Sreeganesh-SchoolsCRUD-Edit branch from 9e371cd to 4bf9017 Compare May 29, 2024 23:32
Copy link
Contributor

@ElijahCanderson ElijahCanderson left a comment

Choose a reason for hiding this comment

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

LGTM

@pconrad pconrad added the FIXME-Merge Conflicts Please fix the merge conflicts then remove this label label May 30, 2024
@SR33G33 SR33G33 removed the FIXME-Merge Conflicts Please fix the merge conflicts then remove this label label May 30, 2024
@pconrad
Copy link
Contributor

pconrad commented Jun 1, 2024

The create school button leads to an error message for me on the dokku deployment linked in the PR.

I can't merge this unless/until I can test it.

@SR33G33
Copy link
Contributor Author

SR33G33 commented Jun 1, 2024

redeployed app and seems to be working on my end, I pushed the most updated working version that has edit and delete and create functionality

note: still has the edit page bugs that I pointed out in the edit page PR which will be fixed in a different PR

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.

Let's clear up the confusion about the use case for these fields.

@pconrad pconrad added the FIXME-See CR Please review the comments in the code review and address them; then remove this label label Jun 1, 2024
@SR33G33 SR33G33 removed the FIXME-See CR Please review the comments in the code review and address them; then remove this label label Jun 1, 2024
@pconrad pconrad added the FIXME-Merge Conflicts Please fix the merge conflicts then remove this label label Jun 2, 2024
@SR33G33 SR33G33 removed the FIXME-Merge Conflicts Please fix the merge conflicts then remove this label label Jun 2, 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.

LGTM

@pconrad pconrad merged commit 4a9de54 into main Jun 5, 2024
9 checks passed
@pconrad pconrad added the 10 label Jun 5, 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.

School Edit Page
5 participants