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 Create and Index Page and Fixed Backend Issues #27

Merged
merged 26 commits into from
Jun 2, 2024

Conversation

SR33G33
Copy link
Contributor

@SR33G33 SR33G33 commented May 28, 2024

Closes #30
Closes #29

Implemented the path for Admin to go to school management page. The NavBar takes the admin to the schools index page

image

Create School button leads to the create page for schools which works and successfully adds the school to the index page and the db

image

Back to index page

image

All course staff and group members should have admin access

dev deployment at this link: https://proj-organic-sr33g33-dev.dokku-12.cs.ucsb.edu/

In the backend, I changed the schoolController to take in a school json object instead of @getparams because of an issue I was facing in testing where the received input was undefined

@SR33G33 SR33G33 changed the title Schools Create and Index Page Schools Create and Index Page and Fixed Backend Issues May 29, 2024
jason-rodrigues

This comment was marked as resolved.

@SR33G33
Copy link
Contributor Author

SR33G33 commented May 29, 2024

I believe the term description needs to follow the regex you inputted, so for your inputted regex the term description would simply be S24, F23

image

Like so

@SR33G33 SR33G33 requested a review from jason-rodrigues May 29, 2024 15:44
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.

LGTM

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

SR33G33 commented Jun 1, 2024

I added this as a bug which I pointed out in my edit page PR, I added it as a new task on the kanban board and am fixing it in a separate PR, I believe that the index and create pages are correct

@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
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 on dokku seems to be working on my end

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.

You cannot delete .env.SAMPLE. It is an essential file for the app.

Also: is it the intention in this PR to have the index page and create feature for school but to have edit and delete both return errors?

That's typically not mergeable. We ask that the button at least link to a placeholder page, or put up a message sayig "not yet implemented".

I might relent in this case, but it's bad practice. So I need clarity on that.

@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
Copy link
Contributor Author

SR33G33 commented Jun 1, 2024

I accidentally deleted the .env.SAMPLE file, I restored it from the version that is on the main branch

The dokku deployment now has the branch version that has the real functionality for edit and delete instead of errors.

I should have inserted placeholder pages however I originally intended to have implemented all 3 functionalities in the same PR but later decided to break up the PR's for organizations sake so I just never got around to putting in placeholders as I just did them one after another.

@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
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.

Looks much better! There is still one issue that we should address: a misunderstanding about the use case / purpose for these fielods.

@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
Copy link
Contributor Author

SR33G33 commented Jun 1, 2024

Fixed it, now TermDescription has nothing to do with the TermRegex and the corresponding tests and checks have been modified/removed, will update the later PRs with this as well

@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
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 3eca1ad into main Jun 2, 2024
14 checks passed
@pconrad pconrad added the 10 label Jun 2, 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 Create Page School Index Page
4 participants