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

integrate decidim-blogs into main repo #3221

Merged
merged 16 commits into from
Apr 20, 2018

Conversation

isaacmg410
Copy link
Contributor

@isaacmg410 isaacmg410 commented Apr 13, 2018

🎩 What? Why?

This PR adds the decidim-blogs into main repo

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry
  • Add decidim-blogs to main repo

📷 Screenshots (optional)

Description

@ghost ghost assigned isaacmg410 Apr 13, 2018
@ghost ghost added the status: WIP label Apr 13, 2018
@isaacmg410
Copy link
Contributor Author

@decidim/lot-core can anyone help me to understand/solve the failed test please?

@mrcasals
Copy link
Contributor

@isaacmg410 it's saying this particular key is missing from the locales file, so you should add it

@isaacmg410
Copy link
Contributor Author

@mrcasals I thought that he was referring to something else. Also, I saw that there was another dot into the index page.

@isaacmg410 isaacmg410 changed the title [WIP] integrate decidim-blogs into main repo integrate decidim-blogs into main repo Apr 16, 2018
@isaacmg410
Copy link
Contributor Author

@decidim/lot-core Ready to review and merge

Copy link
Contributor

@mrcasals mrcasals 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 see any system spec checking that the user can navigate posts and see one of them, can you please add some?

@@ -534,6 +534,34 @@ jobs:
root: coverage
paths:
- codeclimate.sortitions.json
blogs:
Copy link
Contributor

Choose a reason for hiding this comment

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

This job is not being used, you need to add it to the workflow at the end of the file. You'll also need to modify the upload-coverage job to increase the number of the -p parameter, and make that job depend on the blogs one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done!

CHANGELOG.md Outdated
@@ -10,6 +10,7 @@ controller or added a new module you need to rename `feature` to `component`.

**Added**:

- **decidim-blogs**: Decidim Blogs gem has been integrated into the main repository. [\#3221](https://github.com/decidim/decidim/pull/3221)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some weird extra spaces between words here

@mrcasals
Copy link
Contributor

Also, I find it weird that the blogposts have an author. In proposals, meeting and debates, when a resource is created from the admin we show it as "official" and we render the name of the organization instead of the author name, but posts show the name of the user that created it.

Is this intended @decidim/product?

@mrcasals mrcasals mentioned this pull request Apr 17, 2018
4 tasks
@ghost ghost added the status: WIP label Apr 17, 2018
@isaacmg410
Copy link
Contributor Author

@mrcasals I understand that it may seem strange. But is what @decidim/product asked in a face-to-face meeting. The designs are also made with author.

On the other hand, what sense does the author have to be "Official"? So therefore, I think the author field would not be necessary. what do you think?

Another thing is that a post can not be created from the public part, only the admins via admin site...

@mrcasals
Copy link
Contributor

@isaacmg410 ok then 🤷‍♂️

I still can't find any system spec, can you add one them please?

@ghost ghost added the status: WIP label Apr 18, 2018
@isaacmg410
Copy link
Contributor Author

@mrcasals system specs added

@mrcasals
Copy link
Contributor

@isaacmg410 great, thanks! Some specs are failing though, can you check them please?

@isaacmg410
Copy link
Contributor Author

@mrcasals It's strange, the specs complain about the design app and decidim-debates ...

@ghost ghost assigned oriolgual Apr 19, 2018
@ghost ghost added the status: WIP label Apr 19, 2018
Copy link
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@isaacmg410
Copy link
Contributor Author

So, can be merged?

@mrcasals mrcasals merged commit 3e57a14 into master Apr 20, 2018
@mrcasals mrcasals deleted the integrate_module_blogs_to_main_repo branch April 20, 2018 12:49
@mrcasals
Copy link
Contributor

Right after merging this PR, master had a random failure:

https://circleci.com/gh/decidim/decidim/88529

😞

@isaacmg410 isaacmg410 mentioned this pull request Apr 23, 2018
1 task
@isaacmg410
Copy link
Contributor Author

@mrcasals How strange. 😕 I have run the master tests locally and it does not happen to me.
I have created a new PR #3266 changing the title to a fix text. Can you take a look?

@mrcasals mrcasals mentioned this pull request Apr 23, 2018
@xabier
Copy link

xabier commented Apr 25, 2018

sorry to be so late on the response. Just to be clear, how was the issue of blog entry authorship finally managed?

@mrcasals
Copy link
Contributor

@xabier it was left as it is, so no changes were applied.

@xabier
Copy link

xabier commented Apr 25, 2018

@mrcasals thanks, well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants