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

Expand the search range in used/match when querying needles #3181

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

Amrysliu
Copy link
Contributor

@Amrysliu Amrysliu changed the title Expand the search range in used/match when querying needles WIP: Expand the search range in used/match when querying needles Jun 17, 2020
@Amrysliu
Copy link
Contributor Author

The format after modification like this:
needle

I would like to confirm that if this format is acceptable? Any feedback is appreciated.
I also wonder if the time format transfer method is correct. (Change 2020-05-12T12:56:23 to 2020-05-12 12:56:23). Is it better using s/T/ /g ?

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #3181 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3181      +/-   ##
==========================================
+ Coverage   92.07%   92.09%   +0.01%     
==========================================
  Files         213      213              
  Lines       13017    13023       +6     
==========================================
+ Hits        11986    11993       +7     
+ Misses       1031     1030       -1     
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Controller/Admin/Needle.pm 88.23% <100.00%> (+7.59%) ⬆️
lib/OpenQA/Worker/Job.pm 75.20% <0.00%> (-0.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b28dd34...c96bc9c. Read the comment docs.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

I unfortunately do not understand the question about the date format and conversion. You are using "datetime_field" so that already prescribes the display format and I assume that can depend on browser language selections of the user? And for a date transformation I would not use regex search-replace but use proper datetime conversion methods which I think you use correctly already.

</select>
%= datetime_field 'text_custom_last_match', (id => 'last_match_date') => (size => 14) => (class => 'form-control custom_datetime') => (value => $deftime)
<input type="button" id="btn_custom_last_match" class="btn" value="Apply">
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Instead of duplicating for "seen" and "match" you can try to define a helper, e.g. in https://github.com/os-autoinst/openQA/blob/master/lib/OpenQA/WebAPI/Plugin/Helpers.pm and use that in both places with "seen" and "match" as arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean I could define a helper including all seen and match contents, e.g all code in <form>?

Copy link
Contributor

Choose a reason for hiding this comment

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

Writing a custom helper would likely mean making all these elements via Perl code. Including a nested template is likely the better alternative here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed that to use a nested template.

@Amrysliu Amrysliu changed the title WIP: Expand the search range in used/match when querying needles Expand the search range in used/match when querying needles Jun 22, 2020
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

I suppose the HTML layout is ok. There are a few alignment issues but it isn't very important.

Regarding the time format: T is the standard determiner used for combined date and time representations according to ISO 8601. I'd prefer to use that standard but if it is not convenient (e.g. you had to change existing code for consistency or do messy re-formatting) I'd say use whatever works.

templates/webapi/admin/needle/index.html.ep Outdated Show resolved Hide resolved
</select>
%= datetime_field 'text_custom_last_match', (id => 'last_match_date') => (size => 14) => (class => 'form-control custom_datetime') => (value => $deftime)
<input type="button" id="btn_custom_last_match" class="btn" value="Apply">
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Writing a custom helper would likely mean making all these elements via Perl code. Including a nested template is likely the better alternative here.

if (lastSeen === 'custom') {
lastSeen = $('#sel_custom_last_seen').val() + $('#last_seen_date').val();
}
return url + "?last_match=" + encodeURIComponent(lastMatch) + "&last_seen=" + encodeURIComponent(lastSeen);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the URI encoding

@Amrysliu
Copy link
Contributor Author

I suppose the HTML layout is ok. There are a few alignment issues but it isn't very important.

Regarding the time format: T is the standard determiner used for combined date and time representations according to ISO 8601. I'd prefer to use that standard but if it is not convenient (e.g. you had to change existing code for consistency or do messy re-formatting) I'd say use whatever works.

Do I need to modify that? I do not get your point about using that standard. Could you give me more details?

@kalikiana
Copy link
Member

Screenshot from 2020-07-01 11-38-01

This looks very crammed here unless I maximize the window 🤔 It seems that .custom_select { width: 18% } is what's causing that

@Amrysliu
Copy link
Contributor Author

Amrysliu commented Jul 2, 2020

Screenshot from 2020-07-01 11-38-01

This looks very crammed here unless I maximize the window thinking It seems that .custom_select { width: 18% } is what's causing that

yes, I do not use width : 100px because the select will be wide following the window's width.

assets/stylesheets/admin-pages.scss Outdated Show resolved Hide resolved
@kalikiana kalikiana requested review from Martchus and okurz July 7, 2020 12:27
@mergify mergify bot merged commit cdb098b into os-autoinst:master Jul 7, 2020
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.

None yet

4 participants