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

First federation steps #80

Merged
merged 14 commits into from
Oct 10, 2021

Conversation

chagai95
Copy link

@chagai95 chagai95 commented Oct 7, 2021

I disabled CORS and removed permission checks so these things have to be considered and tested to see what the implications are.

Also the colors are different than upstream.

I will try to show a better view when clicking on a federated dot but for now it is erroring

Part of #62

@weex
Copy link

weex commented Oct 7, 2021

What issue does removing CORS headers and permission checks solve?

@chagai95
Copy link
Author

chagai95 commented Oct 7, 2021

Trying to use the api from a different origin fails because of CORS, and since we have no concept for data replication and there were checks to see if the user is logged in I removed those as well.

@weex
Copy link

weex commented Oct 7, 2021

Trying to use the api from a different origin fails because of CORS, and since we have no concept for data replication and there were checks to see if the user is logged in I removed those as well.

I don't really have a problem merging this once it's not a draft, but do consider making issues for these in case other solutions may be possible.

I can imagine someone in the future may look at the project and say "security! they're not even using CORS" and add it back, and if so fine. My feeling is another API testing method may let you supply the correct headers (e.g. Postman).

@chagai95
Copy link
Author

chagai95 commented Oct 7, 2021

Yeah, more documentation would be a good idea.

@chagai95 chagai95 marked this pull request as ready for review October 7, 2021 20:43
@chagai95 chagai95 requested a review from a team as a code owner October 7, 2021 20:43
@mariha
Copy link
Member

mariha commented Oct 10, 2021

I am going to merge it. It may not meet our quality standards but moves us forward in the right direction.

Kids are either clean or happy, I am thinking the same is true for developers and a foss community.

Instead of detailed PR reviews I'd do optimistic merge and post-merge review in order to close an issue. So we'll keep issues open until our functional and quality standards are met, but merge PRs quickly to maximize on contributors positive experience.

Over time we could add automated checks for things that we expect for each code change, like new tests to be added so that the change is protected from being broken in the future.

@mariha mariha merged commit ea8f9ed into OpenHospitalityNetwork:master Oct 10, 2021
mariha pushed a commit that referenced this pull request Feb 16, 2022
* allow all users to make offers.api requests

* temporarily allow all CORS

* avoid checks for authentication until a better concept is made in policy

* concatinate data from the local and 1 remote instance 

* change the colors of the dots

* open openhospitality.network link when clicking on federated dots on map
mariha added a commit that referenced this pull request Mar 6, 2022
mariha pushed a commit that referenced this pull request Mar 13, 2022
* allow all users to make offers.api requests

* temporarily allow all CORS

* avoid checks for authentication until a better concept is made in policy

* concatinate data from the local and 1 remote instance

* change the colors of the dots

* open openhospitality.network link when clicking on federated dots on map

(cherry picked from commit e1e6bed)
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.

3 participants