Obsolete text field "attachment name..." #979

jhyle · 2016-01-28T14:25:59Z

See #865. This patch adds a js function that fills the attachement name field when a file is selected for upload. The user can change the name then. At upload, the name is taken for the attachement if given, the name of the uploaded file otherwise.

I had to add to suppress the ExcessiveImports warning, maybe create a puzzle for it?

karato · 2016-01-28T14:44:24Z

@jhyle Thanks, let me find someone who can review this pull request

karato · 2016-01-28T15:08:22Z

@krzyk review it

krzyk · 2016-01-29T08:24:42Z

netbout-web/src/main/java/com/netbout/rest/bout/TkAttach.java

+     * @param dst File
+     * @throws IOException If fails
+     */
+    private void copy(final Request src, final File dst) throws IOException {


@jhyle instead of creating this method you can reuse Files.copy(src.body, dst.toPath())

krzyk · 2016-01-29T09:14:06Z

@jhyle please see my comments above and also a test would be nice

krzyk · 2016-01-29T09:18:24Z

netbout-web/src/main/java/com/netbout/rest/bout/TkAttach.java

@@ -54,6 +55,7 @@
 import org.takes.facets.forward.RsForward;
 import org.takes.rq.RqHeaders;
 import org.takes.rq.RqMultipart;
+import org.takes.rq.RqMultipart.Smart;


@jhyle don't use such imports, the correct class name should be RqMultipart.Smart, not just Smart

jhyle · 2016-01-29T15:54:45Z

@krzyk Ok, I did the changes and added a puzzle for the missing test.

krzyk · 2016-01-29T16:50:25Z

@jhyle unfortunately we need to have the test first, implementation (or a part of it) could be in todo, but the test that recreates the problem reported has to exist

jhyle · 2016-01-30T06:50:54Z

@krzyk @karato Ok, next time I do the test first and then do a puzzle for the change. Can I have additional 30 min for this one?

jhyle · 2016-01-31T15:57:52Z

@karato ping

jhyle · 2016-02-01T22:51:27Z

@karato Please give an answer.

krzyk · 2016-02-02T07:49:16Z

@jhyle I haven't seen a case where the budget was extended, you could try with Yegor. But basically, when you take a task you accept http://at.teamed.io/policy.html and specifically:

Each task has a pre-defined budget, in minutes, which is announced by the project manager. The budget is not negotiable. When the task is done, you’ll get a payment equal to the number of minutes in the budget multiplied by your hourly rate, no matter how much time you actually spent on it.

and this http://www.yegor256.com/2015/07/16/fools-dont-write-unit-tests.html

karato · 2016-02-03T15:54:49Z

@karato ping

@jhyle no, you can't have additional 30 minutes

jhyle · 2016-02-04T09:06:05Z

@krzyk I added a test, but it fails with

org.takes.HttpException: [400] header "Content-Disposition" is mandatory
    at org.takes.rq.RqHeaders$Smart.single(RqHeaders.java:206)
    at org.takes.rq.RqMultipart$Base.asMap(RqMultipart.java:313)
    at org.takes.rq.RqMultipart$Base.buildRequests(RqMultipart.java:224)
    at org.takes.rq.RqMultipart$Base.<init>(RqMultipart.java:140)
    at org.takes.rq.RqMultipart$Fake.<init>(RqMultipart.java:413)
    at com.netbout.rest.bout.TkAttachTest.setAttachementName(TkAttachTest.java:203)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:459)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:675)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:382)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:192)

Can it be that the RqMultipart.Fake does not handle multiple dispositions correctly?

    /**
     * Take attachement name from name field if value is set.
     * @throws Exception If there is some problem inside
     */
    @Test
    public void setAttachementName() throws Exception {
        final MkBase base = new MkBase();
        final String urn = "urn:test:3";
        final User user = base.user(new URN(urn));
        user.aliases().add("jeff3");
        final Alias alias = user.aliases().iterate().iterator().next();
        final long number = alias.inbox().start();
        final Bout bout = alias.inbox().bout(number);
        bout.friends().invite(alias.name());
        final String name = "test.jpg";
        final String filename = "tt.jpg";
        final RqWithAuth request = new RqWithAuth(
            urn,
            new RqMultipart.Fake(
                TkAttachTest.fake(number),
                new RqWithHeaders(
                    TkAttachTest.body(""),
                    String.format(TkAttachTest.POST_URL, number),
                    //@checkstyle LineLengthCheck (1 line)
                    String.format("Content-Disposition: form-data; name=\"file\"; filename=\"%s\"", filename),
                    "Content-Type: application/octet-stream"
                ),
                new RqWithHeaders(
                    TkAttachTest.body(name),
                    String.format(TkAttachTest.POST_URL, number),
                    "Content-Disposition: form-data; name=\"name\""
                )
            )
        );
        try {
            new FkBout(".++", new TkAttach(base)).route(request);
        } catch (final RsForward response) {
            MatcherAssert.assertThat(
                response,
                new HmRsStatus(HttpURLConnection.HTTP_SEE_OTHER)
            );
        }
        MatcherAssert.assertThat(
            bout.messages().iterate().iterator().next().text(),
            Matchers.containsString(String.format("attachment \"%s\"", name))
        );
    }

krzyk · 2016-02-04T21:14:44Z

@jhyle sorry I don't know, you would have to look at the code of this class

jhyle · 2016-02-05T23:47:59Z

@karato @krzyk waiting for yegor256/takes#577

karato · 2016-02-08T17:02:17Z

@karato @krzyk waiting for yegor256/takes#577

@jhyle OK, take your time

krzyk · 2016-02-17T20:15:03Z

@jhyle maybe you could find another way to test this without use this class?

jhyle · 2016-02-18T12:05:22Z

@krzyk Sorry, but I already invested a lot of time in this task. @karato Maybe it is possible to assign the task yegor256/takes#577 to me, so I can find out what is not working?

krzyk · 2016-03-20T11:05:49Z

@jhyle Are you planning to finish this? Generally tasks should be closed in under 10 days, if you can't do it in such time you should ask for reassignment.

krzyk · 2016-03-21T07:34:31Z

@jhyle Please close this PR if you are not planning to finish.

krzyk · 2016-03-23T08:18:03Z

@jhyle ping

krzyk · 2016-03-27T07:51:26Z

@jhyle please close this issue

krzyk · 2016-03-29T15:45:43Z

@dmzaytsev what should I do when @jhyle is not responding?

dmzaytsev · 2016-03-29T15:50:04Z

@krzyk I think we just ask the PM to make it invalid, but in this case you won't be paid
or maybe @yegor256 can help here ?

krzyk · 2016-03-31T21:02:12Z

@yegor256 what should we do here, @jhyle is not responding, PR is waiting for 2 months. Should it be closed or invalidated?

krzyk · 2016-04-02T07:00:37Z

@jhyle ping

yegor256 · 2016-04-04T19:17:50Z

@dmzaytsev what do you think?

dmzaytsev · 2016-04-05T03:57:45Z

@yegor256 I think you could close this PR due to @jhyle not responding for a while
I wouldn't like ask PM for a invalid label because the reviewer did a great job and should be paid

yegor256 · 2016-04-05T09:32:32Z

@dmzaytsev @krzyk I'm closing

karato · 2016-04-06T14:24:16Z

@ypshenychka please, check this issue for QA compliance, as per par.24

ypshenychka · 2016-04-06T14:33:14Z

@dmzaytsev

dmzaytsev commented 8 days ago
I think we just ask the PM to make it invalid, but in this case you won't be paid
or maybe @yegor256 can help here ?

According to our QA Rules:

Messages in a ticket always start with a name of a user they are addressed to.

Please correct your message by indicating an addressee in the beginning.

dmzaytsev · 2016-04-06T14:35:09Z

@ypshenychka fixed

ypshenychka · 2016-04-06T14:51:58Z

@dmzaytsev Thank you.

ypshenychka · 2016-04-06T14:52:13Z

@karato Quality is acceptable.

karato · 2016-04-06T16:35:11Z

@karato Quality is acceptable.

@ypshenychka thanks, got it. everybody, please try to make it good next time

karato · 2016-04-06T16:51:30Z

@krzyk I added 10 mins to @ypshenychka (for QA review) in transaction 82738942. 32 mins added to your account (payment number 82738977), many thanks for your contribution! 1628 hours and 1 min spent here.. extra minutes for review comments (c=17). +32 added to your rating, at the moment it is: +5666

derive sanitized name for attached files and use it if given

d237864

jhyle mentioned this pull request Jan 28, 2016

Obsolete text field "attachment name..." #865

Closed

karato added CR @krzyk labels Jan 28, 2016

krzyk reviewed Jan 29, 2016
View reviewed changes

Leo added 2 commits January 29, 2016 12:50

use nio for copying attachments + better filename retrieval

901cc52

fix formatting issues

ce94209

Leo added 2 commits February 6, 2016 00:22

test for attaching file with given name

b6865df

fix checkstyle issues

0afb84d

jhyle mentioned this pull request Feb 5, 2016

RqMultiPart.Fake fails on more then one part yegor256/takes#577

Closed

yegor256 closed this Apr 5, 2016

Obsolete text field "attachment name..." #979

Obsolete text field "attachment name..." #979

Conversation

jhyle commented Jan 28, 2016

karato commented Jan 28, 2016

karato commented Jan 28, 2016

krzyk Jan 29, 2016

Choose a reason for hiding this comment

krzyk commented Jan 29, 2016

krzyk Jan 29, 2016

Choose a reason for hiding this comment

jhyle commented Jan 29, 2016

krzyk commented Jan 29, 2016

jhyle commented Jan 30, 2016

jhyle commented Jan 31, 2016

jhyle commented Feb 1, 2016

krzyk commented Feb 2, 2016

karato commented Feb 3, 2016

jhyle commented Feb 4, 2016

krzyk commented Feb 4, 2016

jhyle commented Feb 5, 2016

karato commented Feb 8, 2016

krzyk commented Feb 17, 2016

jhyle commented Feb 18, 2016

krzyk commented Mar 20, 2016

krzyk commented Mar 21, 2016

krzyk commented Mar 23, 2016

krzyk commented Mar 27, 2016

krzyk commented Mar 29, 2016

dmzaytsev commented Mar 29, 2016

krzyk commented Mar 31, 2016

krzyk commented Apr 2, 2016

yegor256 commented Apr 4, 2016

dmzaytsev commented Apr 5, 2016

yegor256 commented Apr 5, 2016

karato commented Apr 6, 2016

ypshenychka commented Apr 6, 2016

dmzaytsev commented Apr 6, 2016

ypshenychka commented Apr 6, 2016

ypshenychka commented Apr 6, 2016

karato commented Apr 6, 2016

karato commented Apr 6, 2016