Skip to content

Skip parameters which are not there instead of failing out - #257

Merged
nickvergessen merged 1 commit into
masterfrom
bugfix/256/skip-on-error
Apr 10, 2018
Merged

Skip parameters which are not there instead of failing out#257
nickvergessen merged 1 commit into
masterfrom
bugfix/256/skip-on-error

Conversation

@nickvergessen

@nickvergessennickvergessen commented Mar 20, 2018

Copy link
Copy Markdown
Member

Fix#256
Fix#77

Testing today:

  1. Switch to italian
  2. Create a file
  3. Share by email

Testing tomorrow:

See above + revert #256 (comment)

Expected

  1. The stream is loaded (broken parameters displayed as is)

Actual

  1. The stream breaks completly.

@nickvergessennickvergessen added this to the Nextcloud 14 milestone Mar 20, 2018
blizzz
blizzz reviewed Apr 4, 2018
View reviewed changes
Comment threadjs/richObjectStringParser.js
parameter = parameter.substring(1, parameter.length - 1);
var parsed = self.parseParameter(parameters[parameter]);
if (!parameters.hasOwnProperty(parameter) || !parameters[parameter]) {
// Malformed translation?

@blizzzblizzzApr 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should this be reported instead of failing silently?

@nickvergessennickvergessenApr 9, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well only thing would be console.log, but that is disabled for non-debug anyway, so I don't think it's important.

@blizzzblizzzApr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so, wouldn't it be helpful for debugging? for the app dev to to find out where an issues occurs?

@nickvergessennickvergessenApr 9, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

whatever I was thinking, yes of course, amended

@blizzz

blizzz commented Apr 4, 2018

Copy link
Copy Markdown
Member

travis is unhapy

@nickvergessen

nickvergessen commented Apr 9, 2018

Copy link
Copy Markdown
MemberAuthor

Travis was fixed with https://github.com/nextcloud/activity/pull/259/files

But I will rebase

@nickvergessen
nickvergessenforce-pushed the bugfix/256/skip-on-error branch from 6de54e7 to 92b9084CompareApril 9, 2018 19:58
@nickvergessen
Skip parameters which are not there instead of failing out
3da098f
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
nickvergessenforce-pushed the bugfix/256/skip-on-error branch from 92b9084 to 3da098fCompareApril 9, 2018 21:05
@nickvergessen
nickvergessen merged commit 91cfac7 into masterApr 10, 2018
@nickvergessen
nickvergessen deleted the bugfix/256/skip-on-error branch April 10, 2018 10:44
@MorrisJobke

MorrisJobke commented Apr 18, 2018

Copy link
Copy Markdown
Member

@nickvergessen Next RC is coming - time for backport

This was referenced Apr 18, 2018
Merged
Merged
@MorrisJobke

MorrisJobke commented Jul 11, 2018

Copy link
Copy Markdown
Member

@nickvergessen To backport or not to backport?

@nickvergessen

nickvergessen commented Jul 11, 2018

Copy link
Copy Markdown
MemberAuthor

ws already backported see #265

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blizzzblizzzblizzz approved these changes
@rullzerrullzerAwaiting requested review from rullzer

Assignees

No one assigned

Labels

3. to reviewbugfeature: stream

Projects

None yet

Milestone

Nextcloud 14

Development

Successfully merging this pull request may close these issues.

3 participants

@nickvergessen@blizzz@MorrisJobke