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

Model validation UI + manifest-aware validation #236

Merged
merged 27 commits into from
Apr 12, 2023

Conversation

gtfierro
Copy link
Collaborator

@gtfierro gtfierro commented Apr 1, 2023

  • fixes docker compose setup, which seemed to be broken. I included the changes in this PR so that the UI can be more easily used during testing
  • Fixes buildingmotif.dataclasses.model.Model.validate so that it defaults to the Model's manifest if no shape collections are provided. Note: to avoid backwards incompatibility, I made the argument "optional". This means that you would do my_model.validate(None) or my_model.validate([]) to default to the model's manifest for validation. IF we are ok with switching Model.validate to be variadic, then this call can be simplified to my_model.validate()
  • incorporate this new behavior into the implementation of the model validation endpoint
  • add tests for the new Model validation endpoint
  • fix tests on the library's get_all_shapes call

Supersedes #214 (now closed)

TODOs:

  • fixup UI to support selecting libraries for validation, not shapes

@gtfierro
Copy link
Collaborator Author

gtfierro commented Apr 3, 2023

@haneslinger I've included #228 and #214 into a new branch which has also had develop merged in. I've also added the feature where calling Model.validate(None) (with no shape collections) defaults to the model's manifest. I can change the signature of validate to be variadic if you want, that way we can just do Model.validate(); this would break existing code, but it's an easy fix.

I've fixed a couple of the API Library tests that were failing before. I still need to address how the model validation call happens from the API

@gtfierro
Copy link
Collaborator Author

gtfierro commented Apr 3, 2023

@haneslinger I've also implemented a version of the model validate endpoint that's included in this PR w/ some passing tests. Let me know what you think!

@gtfierro gtfierro marked this pull request as ready for review April 3, 2023 14:42
@gtfierro gtfierro changed the base branch from validate-on-shapes to develop April 3, 2023 20:13
@gtfierro gtfierro changed the title Updating #214 with develop merge and docker compose fixes Model validation UI + manifest-aware validation Apr 3, 2023
@gtfierro gtfierro marked this pull request as draft April 3, 2023 20:55
Copy link
Collaborator

@haneslinger haneslinger left a comment

Choose a reason for hiding this comment

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

I can't get the docker compose to work for me.. the db config seems off...

buildingmotif-db   | 2023-04-04 13:21:01.217 UTC [4632] FATAL:  role "root" does not exist

We can talk about it later.

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
@gtfierro
Copy link
Collaborator Author

gtfierro commented Apr 4, 2023

I can't get the docker compose to work for me.. the db config seems off...

buildingmotif-db   | 2023-04-04 13:21:01.217 UTC [4632] FATAL:  role "root" does not exist

We can talk about it later.

PR is still a draft so it's not ready for review!

@gtfierro
Copy link
Collaborator Author

gtfierro commented Apr 5, 2023

@haneslinger I believe I've faithfully replicated your shape selector code with libraries, but I'm getting an error in the AngularJS frontend code and I'm having a hard time tracking down why it is happening. The console error is "Cannot find control with unspecified name attribute", but searching online just reveals a bunch of people playing whack-a-mole with different fixes; installing the DevTools extension in Chrome didn't even give me more insight. Would you be able to take a look? As far as I can tell, it has something to do with generating the list correct FormControl objects in the component's HTML

@haneslinger
Copy link
Collaborator

Fixed it. Can't tell you exactly why this works... something something lazy lookup, I think?

@gtfierro
Copy link
Collaborator Author

gtfierro commented Apr 5, 2023

Fixed it. Can't tell you exactly why this works... something something lazy lookup, I think?

Thanks! I'll continue with this -- I don't think I've correctly hooked the validate UI to the endpoint yet

@gtfierro gtfierro marked this pull request as ready for review April 7, 2023 17:09
@gtfierro
Copy link
Collaborator Author

gtfierro commented Apr 7, 2023

Should be ready for review now, @haneslinger -- learned more about Angular to get some of the UI work done (though my work could definitely be improved...)

image

@TShapinsky TShapinsky self-requested a review April 10, 2023 19:37
@TShapinsky
Copy link
Member

@TShapinsky look at the docker stuff.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@haneslinger haneslinger left a comment

Choose a reason for hiding this comment

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

i think we've got it

@gtfierro
Copy link
Collaborator Author

i think we've got it

I had one question on a piece of your feedback. Can you take a look when you get a chance? Thanks! Then we can (finally!) merge :)

@gtfierro gtfierro merged commit b70d134 into develop Apr 12, 2023
@gtfierro gtfierro deleted the gtf-214-update-with-docker-compose branch April 12, 2023 20:34
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.

None yet

3 participants