Fixes issue #1407: language in title #1413

mauditecandela · 2017-09-25T17:13:18Z

ragesoss · 2017-09-25T17:21:15Z

app/assets/javascripts/components/articles/article.jsx

@@ -21,7 +21,7 @@ const Article = React.createClass({

  shouldShowLanguagePrefix() {
    if (!this.props.course.home_wiki) { return false; }
-    return this.props.article.language !== this.props.course.home_wiki.language;
+    return this.props.article.language !== this.props.course.home_wiki.language && null;


I'm not clear about what the && null is doing here. It looks like this would always return either false or null.

yes, you are right. reworking on it right now, sorry.

@ragesoss I have doubled checked and at least in my local database, the articles are not receiving the language parameters, and that's why the behaviour was not as expected. Do you know if that happens in the production database as well?

(Wikimedia project lang is always NULL and also their articles, which was the only case the if statement was returning true - both languages are NULL).

One quickfix for the bug would be to change the if statement to include the language only if article.language !== home_wiki.language. Also I would add the home_wiki language in order not to have any problems with the already mention lack of article languages in the database.

So this line (line 38):

const languagePrefix = this.shouldShowLanguagePrefix() ? ${this.props.article.language}: : '';
would change to:
const languagePrefix = this.shouldShowLanguagePrefix() ? '' : ${this.props.course.home_wiki.language}:;

One bigger task would be to research why the articles have not languages defined in the database.

What do you think? If you agree I will do the new PR.

ragesoss · 2017-09-26T15:43:30Z

@mauditecandela looking this over, I'm thinking that the best approach might be to have every article go through CourseUtils.formattedArticleTitle. That way we only have one place that is responsible for generating formatted article titles. With that approach, it may make sense to add another helper function to CourseUtils that takes a course object and an article object and handles the 'home_wiki' part of the current implementation in article.jsx.

A big benefit to moving it over to CourseUtils will be that it'll be easy to write unit tests for it (in test/utils/course_utils.spec.js).

mauditecandela · 2017-09-26T15:47:18Z

I will try that out.

…tted titles #1407

mauditecandela · 2017-09-27T12:27:22Z

@ragesoss and @testa19 here it is a test. I am not 100% sure if the logic is the desirable, when testing it in my local environment it seemed fine but could be that I am missing something. Could you please have a look and give me some feedback? Thanks a lot.

ragesoss · 2017-09-27T15:39:17Z

@mauditecandela there's another linting error that is breaking the build. If you run gulp locally, it will re-run the linter ever time you save a change so you can see any linting errors quickly. I also recommend installing an eslint plugin to your code editor, so you can see any linting errors as you're codeing.

mauditecandela · 2017-09-27T18:56:41Z

aaaand it's green! Waiting for your feedback :) In my local machine seems to work and worked on the approach of transferring everything to course_utils.js in order to make easier to do some tests.

ragesoss · 2017-09-27T19:21:06Z

@mauditecandela cool!

It looks like the only place that users CourseUtils.formattedArticleTitle is in assignment.jsx, and it already has an article object that will include the language and project.

So I think we should combine those into a single function that takes article and defaultWiki as arguments (which your new function does) and call it from assignment.jsx

I think formattedArticleTitle is a better name for that function. Changing the signature of formattedArticleTitle will cause the tests to break, so you'll need to adjust the tests as well. But it'll also be a good chance to test it more thoroughly and intuitively, around the idea of 'given an article on some wiki, and a default wiki, how do we want the title to be displayed'.

You can run the javascript tests locally by running jest --watch. That will rerun the tests whenever you change a js file or corresponding test.

mauditecandela · 2017-09-28T13:57:46Z

Hi @ragesoss here my first proposal as a possible solution. As you suggested, I have merged both functions but in order to work finely, I also separated the title formatting action also from articleFromAssignment() function. I think this is more consistent, but also lead me to change different files where the article.formatted_title was expected to be available after creating the article from the assignment.

I have also noticed that the problem that appears in this test: https://travis-ci.org/WikiEducationFoundation/WikiEduDashboard/jobs/280854551 was not appearing in my local jest, do you know why?

Btw, thanks for the hint, jest is a great tool and now I love it!

I look forward your feedback :)

ragesoss · 2017-09-28T16:03:17Z

@mauditecandela we have two different test frameworks: Jest for unit testing of Javascript, and RSpec for doing Ruby unit tests as well as browser tests. Those 'multiwiki assignments' tests are browser tests, so they don't get run by Jest. (If you are running guard, it will try to automatically run relevant tests similarly to what Jest does, but because the browser tests aren't linked to a single file, they generally don't get run unless you run them explicitly or you're running the entire test suite.)

ragesoss

I think this will be a lot cleaner if the signature of formattedArticleTitle changes to (article, defaultWiki), where defaultWiki is optional. The more we can have these functions work in terms of the main abstractions used throughout the system — article and wiki, in this case — the easier they will be to understand and maintain.

ragesoss · 2017-09-28T16:20:55Z

test/utils/course_utils.spec.js

@@ -141,7 +141,7 @@ describe('courseUtils.articleFromTitleInput', () => {
  });
 });

-describe('courseUtils.articleFromAssignment', () => {
+describe('courseUtils.formattedArticleTitle', () => {


This is still a test of articleFromAssignment.

ragesoss · 2017-09-28T16:32:07Z

app/assets/javascripts/utils/course_utils.js

@@ -92,18 +92,12 @@ const CourseUtils = class {
    const language = assignment.language || 'en';
    const project = assignment.project || 'wikipedia';
    const articleUrl = assignment.article_url || this.urlFromTitleAndWiki(assignment.article_title, language, project);
-    const formattedTitle = this.formattedArticleTitle(


Instead of removing this and setting formatted_title in other places every time this is called, you could do article.formatted_title = this.formattedArticleTitle(article) before the return.

If necessary, you could also make articleFromAssignment take an optional homeWiki argument to include.

In general, the more shared logic we can move into CourseUtils, the better. Isolating that logic into the shared vanilla Javascript function makes it much easier to test and maintain.

ragesoss · 2017-09-28T16:35:46Z

app/assets/javascripts/utils/course_utils.js

    let languagePrefix = '';
-    if (language === undefined || language === 'en') {
+    if (!home || language === home.language || language === null || language === undefined || language === 'en') {


We can probably do away with the special-casing of 'en' here. Also, see if you can simplify this conditional any further.

ragesoss · 2017-09-28T16:36:52Z

test/utils/course_utils.spec.js

@@ -153,8 +153,6 @@ describe('courseUtils.articleFromAssignment', () => {
    expect(article.url).to.eq('https://es.wikipedia.org/wiki/Autofoto');
    expect(article.title).to.eq('Autofoto');
    expect(article.language).to.eq('es');
-    expect(article.project).to.eq('wikipedia');


This should still be true.

ragesoss · 2017-09-28T18:29:50Z

app/assets/javascripts/components/assignments/assignment.jsx

-        this.props.assignmentGroup[0].project,
-        article.title,
-        this.props.course.home_wiki);
+    article.formatted_title = CourseUtils.formattedArticleTitle(article);


This line seems unnecessary if articleFromAssignment does this already.

ragesoss · 2017-09-28T18:31:25Z

app/assets/javascripts/components/assignments/assignment.jsx

-        this.props.course.home_wiki);
+    article.formatted_title = CourseUtils.formattedArticleTitle(article);
+    if (!article.formatted_title) {
+      article.formatted_title = CourseUtils.formattedArticleTitle(this.props.assignmentGroup[0]);


Here you can do article.formatted_title = CourseUtils.formattedArticleTitle(article, this.course.home_wiki)

The fewer times we need to call assignmentGroup[0], the more understandable this code will be.

ragesoss · 2017-09-28T18:35:01Z

app/assets/javascripts/utils/course_utils.js

-      language,
-      project,
+      language: language,
+      project: project,


This is fine, but means the same thing as what was there before, which is es2015 shorthand notation: { foo } is the same as { foo: foo }.

mauditecandela · 2017-09-28T19:31:37Z

I have already reduced the code as you suggested the only problems that I see are:

Any formatted title created from articleFromAssignment is not showing any language or project. I think it's because they are not defined yet. I think this was the behaviour that I saw at the beginning, can you confirm? Any other suggestions?
There's a test that keeps failing, but I will try to fix it tomorrow :)
Any other feedback is very welcome, of course.

ragesoss · 2017-09-28T19:36:53Z

@mauditecandela I think the way to solve both of these is to pass course.home_wiki articleFromAssignment, and set the language and project from that if it is not provided by the assignment argument.

mauditecandela · 2017-09-29T11:38:05Z

This is passes all tests (finally!) and adds the defaultWiki to articleFromAssignment. I also removed te unneeded part of the object.

ragesoss · 2017-09-29T17:47:23Z

app/assets/javascripts/components/students/assign_button.jsx

@@ -241,7 +241,8 @@ const AssignButton = React.createClass({
      let removeButton;
      let articleLink;
      ass.course_id = this.props.course_id;
-      const article = CourseUtils.articleFromAssignment(ass);
+      const article = CourseUtils.articleFromAssignment(ass, this.props.course.home_wiki);
+      if (!article.formatted_title) { CourseUtils.formattedArticleTitle(article, this.props.course.home_wiki); }


Is this necessary? Unlike in assignment.jsx, this article object is always generated by CourseUtils.articleFromAssignment, so it should always have a formatted_title.

ragesoss · 2017-09-29T17:52:18Z

app/assets/javascripts/utils/course_utils.js

    let languagePrefix = '';
-    if (language === undefined || language === 'en') {
+    if (defaultWiki === undefined || article.language === defaultWiki.language || article.language === null || article.language === undefined) {


These can be simplified somewhat, I think. Instead of checking article.language and defaultWiki against multiple precise conditions, we can just check for falsiness: !defaultWiki and !article.language.

ragesoss · 2017-09-29T18:01:54Z

@mauditecandela this is almost there. I really like how much this simplifies both article.jsx and assignment.jsx.

What remains:

Simplify the logic of the formattedArticleTitle function as much as possible.
Update the course_utils spec to test the various possibilities more thoroughly.

mauditecandela · 2017-09-29T20:30:26Z

Thanks a lot for all the guidance and the comments @ragesoss Here it is the new version. I added some tests and simplified the logic are formattedArticleTitle. Let me know what you think :)

ragesoss · 2017-09-29T20:45:45Z

@mauditecandela the tests still have some room for improvement, to reflect the different situations where article titles get displayed. It looks like everywhere except for components/students/assign_cell.jsx that calls articleFromAssignment() now passes course.home_wiki, but the tests don't reflect that; no defaultWiki is included in the tests for that function. The home_wiki can be added to the call in AssignCell, and the tests should include it. In particular, that test was trying to make sure that when you assign an article from Spanish Wikipedia in the context of an English Wikipedia course, that it shows the language prefix, and that project correctly to Wikipedia.

For the tests of formattedArticleTitle, similarly, it looks like we're going to be able to use it always with the defaultWiki included, so the tests should reflect that, but they should test all the main possible situations: when the article is on the same wiki as the course; when the article is the same project but different language; when the article is same language but different project; when the article is different langauge and different project; and when the article is a different project that doesn't have a language (ie, Wikidata).

mauditecandela · 2017-09-30T19:44:17Z

@ragesoss you were right, I was too tired yesterday and today I saw that the tests were far from accurate. I corrected them, please let me know what you think. I am excited! I enjoyed very much working on this. :)

ragesoss · 2017-09-30T20:38:49Z

test/utils/course_utils.spec.js

+    expect(article.formatted_title).to.eq('wikidata:Judith Butler');
+  });
+
+  it('returns true for a weeks array with trainings', () => {


ragesoss · 2017-09-30T20:42:25Z

I am excited! I enjoyed very much working on this. :)

:-)

Please add the course home_wiki to the AssignCell component, and remove the duplicated hasTrainings() test. I'll test it out on Monday to make sure that everything is behaving as expected, and if so, it should be good to merge.

mauditecandela · 2017-09-30T21:42:55Z

Done!!!

Fixes issue #1407: language in title

4055fb2

ragesoss reviewed Sep 25, 2017

View reviewed changes

mauditecandela added 3 commits September 25, 2017 23:05

fixes the bug #1407 with a better fix - quickfix

43d0f29

fixes the errors reported by travis

68c2567

simpler fix for #1407

3b44127

mauditecandela mentioned this pull request Sep 26, 2017

Article titlle should not say 'null' for Wikidata items, etc #1407

Closed

creates new function in course utils in order to create article forma…

bd4d937

…tted titles #1407

mauditecandela added 3 commits September 27, 2017 18:53

fixes travis error #1407 - deletes a trailing slash

1e4286b

fixes eslint errors

3354dae

fixes problems created with eslint

3fbad5e

mauditecandela added 4 commits September 28, 2017 13:47

more independent solution for #1407·

7127fb0

adds formatted_title to assign_cell

0e5977d

fixes problem with course

fc70a90

Fixes error in multiwiki_assignments

b97e615

ragesoss reviewed Sep 28, 2017

View reviewed changes

includes formatted_title at articleFromAssignment

a2f4e6d

ragesoss reviewed Sep 28, 2017

View reviewed changes

fixes multiwiki spec

6d623d0

mauditecandela added 2 commits September 29, 2017 10:02

adds default wiki to articleFromAssignment)

2553bc9

fixes assign cell problem

558f63c

ragesoss reviewed Sep 29, 2017

View reviewed changes

adds unnecessary if statement

6cc5654

ragesoss reviewed Sep 29, 2017

View reviewed changes

mauditecandela added 2 commits September 29, 2017 21:59

simplifies formattedArticleTitle and adds tests

f52f32a

adds new line at the end of the file

55a0d6b

mauditecandela added 2 commits September 30, 2017 21:17

adds new and better tests for #1407

d1f38c8

fixes syntax error

9a9cc7a

ragesoss reviewed Sep 30, 2017

View reviewed changes

mauditecandela added 2 commits September 30, 2017 23:15

adds default wiki to assign cell and fix bug at tests

597ff03

adds course to proptypes at assign cell

efd3010

ragesoss merged commit f584bfc into WikiEducationFoundation:master Oct 2, 2017

Fixes issue #1407: language in title #1413

Fixes issue #1407: language in title #1413

Uh oh!

Conversation

mauditecandela commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ragesoss commented Sep 26, 2017

Uh oh!

mauditecandela commented Sep 26, 2017

Uh oh!

mauditecandela commented Sep 27, 2017

Uh oh!

ragesoss commented Sep 27, 2017

Uh oh!

mauditecandela commented Sep 27, 2017

Uh oh!

ragesoss commented Sep 27, 2017

Uh oh!

mauditecandela commented Sep 28, 2017

Uh oh!

ragesoss commented Sep 28, 2017

Uh oh!

ragesoss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mauditecandela commented Sep 28, 2017

Uh oh!

ragesoss commented Sep 28, 2017

Uh oh!

mauditecandela commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ragesoss commented Sep 29, 2017

Uh oh!

mauditecandela commented Sep 29, 2017

Uh oh!

ragesoss commented Sep 29, 2017

Uh oh!

mauditecandela commented Sep 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ragesoss commented Sep 30, 2017

Uh oh!

mauditecandela commented Sep 30, 2017

Uh oh!

Uh oh!

mauditecandela commented Sep 29, 2017 •

edited

Loading